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

Unify param names #77

Closed
wants to merge 6 commits into from
Closed

Unify param names #77

wants to merge 6 commits into from

Conversation

DrLachie
Copy link
Contributor

Deals with issue #73

I've changed psf_num_iter to decon_num_iter.
I've also left in the CLI_PARAM_MAP as it deals with things like roi_list going inside the crop parameter for example. Tested a bunch of permutations and seems to behave as expected.

@DrLachie DrLachie linked an issue Aug 29, 2024 that may be closed by this pull request
@DrLachie
Copy link
Contributor Author

Going to need to change the tests as the cli and decon tests will fail for the new parameter names

@multimeric
Copy link
Collaborator

Nice, this is much cleaner. I can see that you also actioned the change to Optional[Tuple] now that it has been fixed upstream.

Can you also pull from master to simplify the diff please.


roi_list: List[Path] = field_from_model(CropParams, "roi_list", description="A list of paths pointing to regions of interest to crop to, in ImageJ format."), #Option([], help="A list of paths pointing to regions of interest to crop to, in ImageJ format."),
roi_subset: List[int] = field_from_model(CropParams, "roi_subset"),
z_range: Optional[Tuple[int,int]] = Option(None, help="The index of the first Z slice to use. All prior Z slices will be discarded.", show_default=False),
Copy link
Collaborator

@multimeric multimeric Aug 29, 2024

Choose a reason for hiding this comment

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

We will need to update the help here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Documentation should say the index of first and last slice slice, right?
@DrLachie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's correct - I'll double check the help descriptions and update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated descriptions - I also added an extra logging line that includes the filename to be processed which is handy sometimes

@multimeric
Copy link
Collaborator

Looks good. Just noting that the field_from_model function I created copies the help from the Pydantic model if it exists. If the CLI usage and the Python usage are the same, I prefer to keep the description in one place. For example for devonvolution iterations:

psf_num_iter: NonNegativeInt = Field(
default=10,
description="Number of iterations to perform in deconvolution"
)

The description= argument is for the arguments that behave differently in the CLI, like physical_pixel_sizes, because it's helpful to explain to the user how to pass a tuple via the CLI. For the Python API this is self-evident.

@multimeric
Copy link
Collaborator

@DrLachie

@DrLachie
Copy link
Contributor Author

DrLachie commented Sep 6, 2024

Just so I'm clear, I remove the descirptions from the options in main and have it pull those from the models which is the preferable place for them?

I see some inputs have a help rather than descirption which doesn't behave the same way?

@multimeric
Copy link
Collaborator

multimeric commented Sep 6, 2024

Yes that's right. So the field_from_model tries to convert a Pydantic Field into a CLI Option.

  • field_from_model(...) copies a model field and uses its description verbatim
  • field_from_model(..., description=) lets you copy the field but change the description to make it CLI specific
  • field_from_model(..., extra_description=) lets you append CLI-specific help in addition to the field's description
  • Option(help=) doesn't copy from anywhere. The reason I used this in the past is because we had some CLI args like z_start that didn't correspond to a model field. I want to avoid doing this if possible, because it duplicates logic from the Pydantic model

@multimeric
Copy link
Collaborator

Closing as #82 has an updated version of this.

@multimeric multimeric closed this Sep 26, 2024
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.

Make the Python API and CLI parameters have uniform names
3 participants