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

Harmonise napari plugin and CLI entry points #193

Merged
merged 3 commits into from
Apr 30, 2024
Merged

Conversation

adamltyson
Copy link
Member

@adamltyson adamltyson commented Apr 30, 2024

Description

This PR makes sure the outputs of the napari plugin and CLI are equal. Previously, as highlighted in #192 by @AdrianAriasAbreu, there were differences when running the same data in the napari plugin and CLI. +

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
The plugin and CLI should be two ways of achieving the same thing.

What does this PR do?

  1. It ensures that the default parameters in the GUI are the same, are are displayed the same as the defaults in the CLI. Previously, some were made negative under the hood. This PR corrects this so everything matches.
  2. It downsamples data in the exact same way as brainglobe_utils.image_io.load.load_any() (as used by the CLI).

References

Closes #192 (I think)

How has this PR been tested?

The same data has been registered with the plugin and CLI and results confirmed to be the same. Existing tests pass.

Is this a breaking change?

No

Does this PR require an update to the documentation?

No, but I noticed some errors in the docs, updated in brainglobe/brainglobe.github.io#170

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@adamltyson
Copy link
Member Author

I assume the tests are failing on macOS as those runners have recently been updated to ARM. #191 should fix this.

Copy link
Member

@IgorTatarnikov IgorTatarnikov left a comment

Choose a reason for hiding this comment

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

Is there a sensible minimum for the smoothing parameters? As it stands right now I can't go below -1.0, or -10. Could be worth adding a min=-99.0 or something similar to the appropriate entries of DEFAULT_PARAMETERS.

@adamltyson
Copy link
Member Author

Good catch, thanks. I've set these to -99 which seems about right.

Copy link
Member

@IgorTatarnikov IgorTatarnikov left a comment

Choose a reason for hiding this comment

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

Looks good! Should we merge now or wait for #191?

@adamltyson
Copy link
Member Author

I think merge. This shouldn't (famous last words) affect only one OS.

@adamltyson adamltyson merged commit 8012442 into main Apr 30, 2024
11 of 12 checks passed
@adamltyson adamltyson deleted the harmonise_entry_points branch April 30, 2024 15:57
@adamltyson
Copy link
Member Author

Will hold off releasing until #191 though.

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.

[BUG] The GUI does not allow negative values in key parameter boxes
2 participants