Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue setup small fix + optional meas_pattern #45

Merged
merged 10 commits into from
May 7, 2022

Conversation

DavidMetzIMT
Copy link
Contributor

@DavidMetzIMT DavidMetzIMT commented May 6, 2022

Hy @liubenyuan,

I saw you merged my precedent PR. could you already test it?
However I saw a small issue by trying to not pass ex_mat or perm to following method!
solve, solve_eit, compute_jac and compute_b_matrix.

  • I solve it by setting them in the scope of each method (instead in _compute_potential_dist which do not return them ...)
  • I add in the test_fem.py a small tests to see if defauflt call of each methods do not throughout an error!

Moreover I have implemented an option to pass throught **kwargs a custom meas_patttern to solve_eit, compute_jac and compute_b_matrix and to EitBase!

  • I added test for that in test_fem.py
  • and I add some notes

davmetz added 10 commits May 6, 2022 16:25
- solve, solve_eit, compute_jac and compute_b_matrix allow topass None ex_mat and None perm so we need to set them in each scope of those methods
- notes: they are afterwards not memory in forward (only in eitbase)
- need to be tested and documented
- add the default  call (without arg) of solve, solve_eit, compute_jac, compute _b
-add @ liubenyuan please can you compute b_truth
- in fem.py improve the error str
@liubenyuan
Copy link
Collaborator

liubenyuan commented May 7, 2022

The voltage_meter might fail for eit_stack_jac.py. I think voltage_meter should be part of a measurement protocol and this function should favor clarity rather than speed. The diff_op can be calculate and initialized once every EITForward class by passing ex_mat, step and parser (as these parameters are all protocol related for a EIT system).

I am rewriting the interface for EITForward class and these apis may subject to changes. We can discuss on this.

More detail:

Forward(mesh, el_pos, ref_el) -> class
  -> solve
  -> EITForward(ex_mat, step, parser) -> class
      -> solve_eit
      -> jac, b

@DavidMetzIMT
Copy link
Contributor Author

DavidMetzIMT commented May 7, 2022

Hy,

The voltage_meter might fail for eit_stack_jac.py. I think voltage_meter should be part of a measurement protocol and this function should favor clarity rather than speed. The diff_op can be calculate and initialized once every EITForward class by passing ex_mat, step and parser (as these parameters are all protocol related for a EIT system).

you mean eit_static_jac.py? I couldn't see any pb can you say more about that?

I am rewriting the interface for EITForward class and these apis may subject to changes. We can discuss on this.

More detail:

Forward(mesh, el_pos, ref_el) -> class
  -> solve
  -> EITForward(ex_mat, step, parser) -> class
      -> solve_eit
      -> jac, b

you want to make a base class Forward and make and child class EitForward?
solve need also ex_mat, and the call of EITForward would be:

EITForward(mesh, el_pos, ref_el, ex_mat, step, parser)

by the way mesh, el_pos, ref_el shoul be part of a mesh class (I started this but I wanted first you to get this fixs)... and ex_mat, step, parser could be part of a stimulation class like in EIDORS! or actually it would be better to pass ex_mat and meas_pattern instead ex_mat, step, parser and do like for ex_mat an external function to compute meas_pattern

Something like that:

by passing objs to to forward you avoid increasing the nb of arg and by need you can add a parameter simply by adding it in the parameter class!
also these classes can o much more on the data they contain (check them, have additional prop relative to the data they contain, see PyEITMesh)

@dataclass
class PyEITMesh
     node
     element
     perm
     el_pos
     ref_el

    @proprety
    def n_nodes:
    @proprety
    def n_el:

ex_mat= eit_scan_lines(n_el, step)

meas_mat= voltage_meter(ex_mat, step, parser)
 
@dataclass
class PyEITStim
     ex_mat
     meas_mat

class Forward(mesh: PyEITMesh, stim: PyEITStim)
    mesh
    stim
    def solve:
    def solve_eit:
    def jac:
    def b:

for these PR I would invite you to add the small fix and the additional option to add a custom meas_pattern is very usefull for special electrode array!

did you already made some changes? let me know about that! can you push it on a test branch? so that we can work together on that?

@liubenyuan
Copy link
Collaborator

sure, I have modify these change based on your previous commit. I will merge your this commit and push to a dev branch so we can work together.

solve (Forward class) only need to know one stimulation pattern (ex_line). And only in this way it would be much easier to port to the complete electrode model.

solve_eit, compute_jac and smear (EITForward class) needs to know the complete protocol (stimulation patterns, ex_mat), step and parser. ex_mat specifies the stimulation, while step and parser forms the measurement pattern. Currently we do not need (or do we?) to combine step and parser into meas_pattern, as it would need a wrapper function to generate a meas_pattern for a EIT system.

@liubenyuan liubenyuan merged commit f710fe7 into eitcom:master May 7, 2022
@DavidMetzIMT
Copy link
Contributor Author

DavidMetzIMT commented May 7, 2022

solve_eit, compute_jac and smear (EITForward class) needs to know the complete protocol (stimulation patterns, ex_mat), step and parser. ex_mat specifies the stimulation, while step and parser forms the measurement pattern. Currently we do not need (or do we?) to combine step and parser into meas_pattern, as it would need a wrapper function to generate a meas_pattern for a EIT system.

I think it is a good idee only to pass ex_mat and meas_pattern and the wrapper function is voltage_meter! So you give the possibility to the user to use your meas_pattern or a custom one! actually like ex_mat!

@DavidMetzIMT
Copy link
Contributor Author

Ok send me a message when have created the dev branch

@liubenyuan
Copy link
Collaborator

@DavidMetzIMT The latest changeset are commit to the dev branch. And please also see issue #44, #46

@DavidMetzIMT DavidMetzIMT deleted the issue_setup_small_fix branch May 10, 2022 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants