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

Refactor core-library modules #600

Closed
tcompa opened this issue Oct 30, 2023 · 8 comments · Fixed by #631
Closed

Refactor core-library modules #600

tcompa opened this issue Oct 30, 2023 · 8 comments · Fixed by #631
Labels

Comments

@tcompa
Copy link
Collaborator

tcompa commented Oct 30, 2023

lib_* prefix is a leftover from when core-library and tasks where in the same namespace, but I think it now just makes every import longer with no real benefit.

Such a change is not at all urgent, but I also think it will only break the packages that do depend on fractal-tasks-core (ref #599) and then it should be fairly harmless.

@jluethi
Copy link
Collaborator

jluethi commented Oct 30, 2023

I’m fine with the idea, but let’s combine it with any other reorg needed. For example: do we want to make some core functions importable from the top level?
Any other changes like reprg of the current lib files (are there any that should be merged or split?) would also make sense.

In that way, task packages then only need to update once.

Besides combining this, in favor of changing sooner rather than later! Because the number of packages may grow soon

@tcompa
Copy link
Collaborator Author

tcompa commented Oct 30, 2023

I’m fine with the idea, but let’s combine it with any other reorg needed.

Fully agreed.

My first guess is that we may refactor the tables-related package structure, based on the shape that V1 specs will take (#582).

For example: do we want to make some core functions importable from the top level?

I'm generally in favor. Let's find out for which functions it would make sense.

@tcompa
Copy link
Collaborator Author

tcompa commented Dec 11, 2023

Based on our call last week with @jluethi, here's a first proposal of how to refactor current core-library modules (I'm still trying to improve some details):

  • fractal_tasks_core.ome_zarr: broad-scope subpackage with all what is related to ome-zarr
    • fractal_tasks_core.ome_zarr.channels
    • fractal_tasks_core.ome_zarr.input_models
    • fractal_tasks_core.ome_zarr.ngff -> we may want a better naming for this one
    • fractal_tasks_core.ome_zarr.masked_loading
    • fractal_tasks_core.ome_zarr.pyramids
    • fractal_tasks_core.ome_zarr.read_fractal_metadata -> we may want a better name
    • fractal_tasks_core.ome_zarr.write -> split this into two parts, with one specifically related to labels
    • fractal_tasks_core.ome_zarr.utils -> if possible, let's find a more specific name
  • fractal_tasks_core.yokogawa -> subpackage with all helper functions which are specific for the Yokogawa converted (@jluethi: is this the name we want to use?)
    • fractal_tasks_core.yokogawa.glob -> rename it to avoid conflicts with glob library (best candidate: filenames)
    • fractal_tasks_core.yokogawa.filename_metadata -> merge with filenames
    • fractal_tasks_core.yokogawa.metadata_parsing
  • fractal_tasks_core.upscale_array: single module
  • fractal_tasks_core.roi: subpackage with all what is currently in lib_regions_of_interest
  • fractal_tasks_core.tables: subpackage related to tables, in principle unrelated to the fact that we use tables to store ROIs of Zarr arrays

@tcompa tcompa changed the title Remove lib_ prefix from core-library module names Refactor core-library modules Dec 11, 2023
@tcompa tcompa linked a pull request Dec 11, 2023 that will close this issue
1 task
@tcompa
Copy link
Collaborator Author

tcompa commented Dec 11, 2023

Some relevant constraints that we can try to enforce with the new structure (and which are valid as of #631 (comment)):

  • __FRACTAL_TABLE_VERSION__ is only used within fractal_tasks_core.table
  • __OME_NGFF_VERSION__ is only used within fractal_tasks_core.ome_zarr and fractal_tasks_core.tasks
  • zarr is only imported within fractal_tasks_core.ome_zarr and fractal_tasks_core.tasks; this is not strictly true, since it leaks into:
    • fractal_tasks_core.roi.is_ROI_table_valid, where we validate both the ROI contents and the Zarr attributes --> maybe we can refactor this one?
    • in fractal_tasks_core.tables (but only for type hints)
  • we don't import from ..: this is currently broken in tables, by from ..ome_zarr.write import _write_elem_with_overwrite

@jluethi
Copy link
Collaborator

jluethi commented Dec 12, 2023

fractal_tasks_core.yokogawa -> subpackage with all helper functions which are specific for the Yokogawa converted (@jluethi: is this the name we want to use?)

Let's go with fractal_tasks_core.cellvoyager, because Yokogawa is the broad brand, we're only covering their cellvoyager microscopes

fractal_tasks_core.yokogawa.glob -> rename it to avoid conflicts with glob library (best candidate: filenames)

Let's go with fractal_tasks_core. cellvoyager.glob

fractal_tasks_core.ome_zarr.ngff -> we may want a better naming for this one

Can we describe the scope of this part?

fractal_tasks_core.ome_zarr.read_fractal_metadata -> we may want a better name

Can we describe the scope of this part?

Some relevant constraints that we can try to enforce with the new structure

Nice constraints!

fractal_tasks_core.roi.is_ROI_table_valid, where we validate both the ROI contents and the Zarr attributes --> maybe we can refactor this one?

Not necessary for the 0.14.0 release. Who should be checking the attrs in the end? I can see that it would come from a function in fractal_tasks_core.tables to load the attrs, but we also didn't really want to have it there, right?

we don't import from ..: this is currently broken in tables, by from ..ome_zarr.write import _write_elem_with_overwrite

+1!

@jluethi
Copy link
Collaborator

jluethi commented Dec 12, 2023

Not necessary for the 0.14.0 release. Who should be checking the attrs in the end? I can see that it would come from a function in fractal_tasks_core.tables to load the attrs, but we also didn't really want to have it there, right?

I guess it could come from the proposed read_table function :)
#632

@tcompa
Copy link
Collaborator Author

tcompa commented Dec 12, 2023

fractal_tasks_core.yokogawa.glob -> rename it to avoid conflicts with glob library (best candidate: filenames)

Let's go with fractal_tasks_core. cellvoyager.glob

I already moved to fractal_tasks_core.cellvoyager.filenames, since the two legacy modules (the one for filename parsing and the one for globbing) are somewhat related (as in "they both act on filenames of CellVoyager-generated image files, either by parsing of by filtering them").

(side remark: I'd tend to avoid using glob.py, because of the confusion between from .glob and from glob.)

@tcompa
Copy link
Collaborator Author

tcompa commented Dec 13, 2023

The main refactor result is now described at https://fractal-analytics-platform.github.io/fractal-tasks-core/version_updates/v0_14_0/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants