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

Define table specifications #582

Merged
merged 35 commits into from
Nov 2, 2023

Conversation

tcompa
Copy link
Collaborator

@tcompa tcompa commented Oct 19, 2023

Close #529
Close #587
Close #588

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md

@tcompa tcompa linked an issue Oct 19, 2023 that may be closed by this pull request
3 tasks
@github-actions
Copy link

github-actions bot commented Oct 19, 2023

Coverage report

The coverage rate went from 90.47% to 90.48% ⬆️
The branch rate is 84%.

None of the new lines are part of the tested code. Therefore, there is no coverage data about them.

@tcompa
Copy link
Collaborator Author

tcompa commented Oct 23, 2023

@jluethi a first draft of the new docs page is taking form.

Any high-level feedback on the layout and contents is already welcome, while I'd suggest that for a more detailed review you can directly work on the source code in this branch.

Here are examples of how to serve the documentation locally, to have the new page visible at http://localhost:8001/roi_tables:

git checkout 529-write-docs-page-for-fractal-tasks-core-rois-v1

# With poetry
poetry install --with docs --all-extras
poetry run mkdocs serve --config-file mkdocs.yml --dev-addr localhost:8001

# Without poetry
python -m venv myenv
source myenv/bin/activate
pip install -r docs/doc-requirements.txt
mkdocs serve --config-file mkdocs.yml --dev-addr localhost:8001

@tcompa tcompa requested a review from jluethi October 23, 2023 15:18
@tcompa tcompa marked this pull request as ready for review October 24, 2023 12:36
Copy link
Collaborator

@jluethi jluethi left a comment

Choose a reason for hiding this comment

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

Great first version @tcompa
Let's discuss at our call. A few bigger points:

Is it the doc for ROI tables or tables in general?
Title is ROI, but text says general table spec. Let's briefly discuss at our call
In short, I see 2 equivalent use-cases:

  • ROIs (and sub-cases): Each row is a region of interest
  • Measurements: Each row is a measured object (e.g. related to a given label in the label image)

If our z_micrometer are not used in micrometers, but as arbitrary units: That's actually a bug. We just never noticed, because many users image with z pixel size of 1. But some users image with 0.6 or 2.0

docs/roi_tables.md Outdated Show resolved Hide resolved
docs/roi_tables.md Outdated Show resolved Hide resolved
docs/roi_tables.md Outdated Show resolved Hide resolved
docs/roi_tables.md Outdated Show resolved Hide resolved
docs/roi_tables.md Outdated Show resolved Hide resolved
docs/roi_tables.md Outdated Show resolved Hide resolved
docs/roi_tables.md Outdated Show resolved Hide resolved
@jluethi
Copy link
Collaborator

jluethi commented Oct 25, 2023

Outlook: Axis generalization

@jluethi
Copy link
Collaborator

jluethi commented Oct 25, 2023

Link to PR for fusing FOVs to single well:
ome/ngff#137

Nomenclature:
Tiled (instead of stitched or fused)

@jluethi
Copy link
Collaborator

jluethi commented Oct 25, 2023

TODO: Mention masked loading functionality

@jluethi
Copy link
Collaborator

jluethi commented Oct 25, 2023

Fractal core Tables (subset of proposed table spec)

3 tables:
ROI tables
Advanced ROI tables (=> naming TBD)
Feature tables (=> should in future potentially add additional metadata, as done in #333 )

@tcompa tcompa changed the title Define ROI-table V1 Define table specifications Oct 27, 2023
Copy link
Collaborator

@jluethi jluethi left a comment

Choose a reason for hiding this comment

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

Looks great, I really like the reworked version @tcompa . Great job!

Only open remaining question is about wording on whether Z is used for well & FOV ROIS, let's briefly discuss tomorrow. The rest are minor wording suggestions or typos :)

docs/tables.md Outdated Show resolved Hide resolved
docs/tables.md Outdated Show resolved Hide resolved
docs/tables.md Outdated Show resolved Hide resolved
docs/tables.md Outdated Show resolved Hide resolved
docs/tables.md Outdated Show resolved Hide resolved
docs/tables.md Show resolved Hide resolved
docs/tables.md Outdated Show resolved Hide resolved
docs/tables.md Outdated Show resolved Hide resolved
docs/tables.md Show resolved Hide resolved
docs/tables.md Outdated Show resolved Hide resolved
@tcompa
Copy link
Collaborator Author

tcompa commented Nov 2, 2023

Thanks for the review.
I addressed all comments but one via 43a622e, feel free to double-check.

@tcompa tcompa merged commit 8a8fa41 into main Nov 2, 2023
17 checks passed
@tcompa tcompa deleted the 529-write-docs-page-for-fractal-tasks-core-rois-v1 branch November 2, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants