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

update API #6

Closed
wants to merge 1 commit into from
Closed

update API #6

wants to merge 1 commit into from

Conversation

casperdcl
Copy link
Member

  • use 'image_to_parametric_files' instead of 'image_to_parametric'
  • add the use of a mask file, can automate the mask file selection according to the current image file organisation
  • change 'user_inputs', split linear_phase_start/end for logan_ref, logan_ref_k2p, mrtm, mrtm_k2p
  • use 'models' (list of 'model's) instead of 'model'

closes JJiao#2

- use 'image_to_parametric_files' instead of 'image_to_parametric'
- add the use of a mask file, can automate the mask file selection according to the current image file organisation 
- change 'user_inputs', split linear_phase_start/end for logan_ref, logan_ref_k2p, mrtm, mrtm_k2p
- use 'models' (list of 'model's) instead of 'model'
@casperdcl casperdcl mentioned this pull request May 31, 2022
@casperdcl casperdcl marked this pull request as ready for review May 31, 2022 10:56
# w=None, r1=1, k2p=0.000250, beta_lim=None, n_beta=40, linear_phase_start=500,
# linear_phase_end=None, km_outputs=None, thr=0.1, fig=False):

def kinetic_models(src, dst=None, params=None, models=['srtmb_basis','logan_ref'], input_interp_method='linear',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about this @JJiao...

  • why try to do multiple models in one function? Why not just call the function multiple times (once per model)?
  • linear_phase_(start|end)_(l|l2|m|m2) seems very clunky (again, old API seems much cleaner)

Copy link
Member Author

@casperdcl casperdcl Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better to do this? That way we have both versions...

def kinetic_modelS(modelS=[...], **kwargs):
    resultS = []
    for model in modelS:
        resultS.append(kinetic_model(**kwargs))
    return resultS

kinetic_modelS vs kinetic_model:

  • pro: easier to select multiple models
  • con: lots of kwargs are unrelated to the selected models, so very confusing, hard to explain, error-prone (e.g. model=['srtmb_basis'], linear_phase_start_l=... doesn't make sense)

Overall I think the con is much bigger than the pro.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Let's use 'kinetic_modelS'

@JJiao
Copy link
Member

JJiao commented May 31, 2022 via email

@casperdcl casperdcl mentioned this pull request May 26, 2023
6 tasks
@casperdcl
Copy link
Member Author

I don't have push access to https://github.com/JJiao/NiftyPAD/tree/patch-3, so merged manually in e3f6221 instead

@casperdcl casperdcl closed this Jun 21, 2023
@casperdcl
Copy link
Member Author

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.

2 participants