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

Drop tool extra #380

Closed
tcompa opened this issue May 31, 2023 · 7 comments · Fixed by #384
Closed

Drop tool extra #380

tcompa opened this issue May 31, 2023 · 7 comments · Fixed by #384
Labels
High Priority Current Priorities & Blocking Issues package

Comments

@tcompa
Copy link
Collaborator

tcompa commented May 31, 2023

We currently have a tool extra

[tool.poetry.extras]
tools = ["matplotlib"]

which we should remove.

Depending on how we structure things in #378 and #379, we may want to include a tools submodule in the package, but without any guarantees on its dependencies. Note that it should then be excluded from the coverage measure.

@tcompa tcompa added package High Priority Current Priorities & Blocking Issues labels May 31, 2023
@tcompa
Copy link
Collaborator Author

tcompa commented Jun 5, 2023

As part of #384, I moved the only existing script (lib_metadata_checks.py) into the dev subpackage, and I will now remove the tools package extra. Note that this means that using that script will fail if the user does not have matplotlib installed.

@tcompa
Copy link
Collaborator Author

tcompa commented Jun 6, 2023

Note that this means that using that script will fail if the user does not have matplotlib installed.

After re-thinking about this, we would rather prefer to fully remove the plotting part from lib_metadata_checks.py. It's definitely not a best practice to have a script with an import that is not installed when installing the package, and if we ever need to rely on run_overlap_check (as we do in some tests) then something may fail in an unexpected way.

Higher-level: this is currently our only example of a "tool", although we may have more in the future. As long as they remain very lightweight, and with no implicit dependencies, we can keep them in the main package. If they become more complex, we should put back a tools extra (and a subpackage).

@tcompa tcompa reopened this Jun 6, 2023
@jluethi
Copy link
Collaborator

jluethi commented Jun 6, 2023

Hmm...
I'm not happy with fully deleting the plotting functionality, as it will be useful to have this available somewhere when more users in the Pelkmans lab with custom microscope acquisition modes start using Fractal.

I see a few ways forward:

  1. We import matplotlib only within the check_well_for_FOV_overlap and _plot_rectangle function and keep the script.
  2. We move this whole script (or at least anything using plotting) outside the fractal-tasks-core repo
  3. We actually make it part of a tool and add the tools extra again after all. Plus move the run_overlap_check somewhere else, so it's not intermixed with this tool (but just an actual lib function).

@jluethi
Copy link
Collaborator

jluethi commented Jun 6, 2023

I thought about it some more, I think we should do the following: Separate the current lib_metadata_checks.py file into library parts & examples:
The run_overlap_check is in the library, e.g. in the lib_regions_of_interest.py file.
And the other 2 functions go into examples => check_well_for_FOV_overlap & _plot_rectangle, go into an example script.

If the example does come with the need to install an extra dependency, so be it (and we'll add a comment to a small README for it). But all the functions in the library can be used fine.

@tcompa
Copy link
Collaborator Author

tcompa commented Jun 6, 2023

I thought about it some more, I think we should do the following: Separate the current lib_metadata_checks.py file into library parts & examples: The run_overlap_check is in the library, e.g. in the lib_regions_of_interest.py file. And the other 2 functions go into examples => check_well_for_FOV_overlap & _plot_rectangle, go into an example script.

If the example does come with the need to install an extra dependency, so be it (and we'll add a comment to a small README for it). But all the functions in the library can be used fine.

This seems a good solution:

  • run_overlap_check remains available in the package - as it is reasonable. We can still use it in the tests, as we currently do.
  • examples is indeed the right place for this kind of scripts/files, which are not shipped via pypi but only available to users who clone the repo. These are not guaranteed to work out of the box: they may lack a dependency, or sometimes they may even be out of date. We can of course make it clear in a README.

(minor detail: notice that we cannot simply split the file into two files that go in different folders, because run_overlap_check uses check_well_for_FOV_overlap, but I'll make it work with the minimal possible redundancy)

@tcompa
Copy link
Collaborator Author

tcompa commented Jun 6, 2023

3142f6e introduces a working version of what is discussed above. We can then polish it as much as needed, but this is enough to continue with #390.

@tcompa tcompa closed this as completed in 3142f6e Jun 6, 2023
@jluethi
Copy link
Collaborator

jluethi commented Jun 6, 2023

That's great and looks like a very clean solution indeed. Bonus points for even having included the two example pdfs to see the output!
Thanks a lot @tcompa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority Current Priorities & Blocking Issues package
Projects
None yet
2 participants