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

Additional axes bounds as semi-open interval #7547

Merged
merged 13 commits into from
Jan 17, 2024
Merged

Additional axes bounds as semi-open interval #7547

merged 13 commits into from
Jan 17, 2024

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Jan 11, 2024

We decided to change the format for additionalAxes in the datasource-properties: the upper bound is now exclusive. That is now also used for volume annotations and in the NML format.

Steps to test:

  • download nd test dataset from notion, put in your binaryData dir
  • run migration script, should update the json (bounds should now be [0,18])
  • open datast in wk, should be able to load data from t=0 to (inclusive) t=17
  • create annotation for the ND datast, download, reupload, should look the same, have correct bounds

TODOs:

  • Naming
    • bounds array with two elements
    • in NML: start, end
  • Backend
    • NGFF explore
    • data loading
    • volume download
      • Also fix mag regex in volume upload (unrelated bug)
  • Frontend
    • slider
    • data loading
  • Migration
    • python script to migrate existing datasource-properties.jsons, traversing binaryData directory
    • migrate volume tracings? Decided not to (there aren’t any on our wk instances)
  • Libs

Issues:


@fm3 fm3 changed the title WIP: Additional axes bounds as semi-open interval Additional axes bounds as semi-open interval Jan 16, 2024
@fm3 fm3 marked this pull request as ready for review January 16, 2024 10:36
@fm3
Copy link
Member Author

fm3 commented Jan 16, 2024

@markbader could you comment if this works with scalableminds/webknossos-libs#966 or if you need anything else from us to adapt it? :)

CHANGELOG.unreleased.md Outdated Show resolved Hide resolved
Co-authored-by: frcroth <frcroth@users.noreply.github.com>
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

front-end also looks good to me 👍

I guess it's not worth it to make the migration idempotent?

tools/migrate-axis-bounds/migration.py Outdated Show resolved Hide resolved
@fm3
Copy link
Member Author

fm3 commented Jan 17, 2024

I guess it's not worth it to make the migration idempotent?

Shouldn’t think so. If it does fail, we have the .bak files. There aren’t hundreds of nd datasets in the first place

@markbader
Copy link

@fm3 For wklibs these changes are fine, and I currently don't need anything else to be adapted. Thanks!

@fm3 fm3 merged commit e806b44 into master Jan 17, 2024
2 checks passed
@fm3 fm3 deleted the axes-semi-open branch January 17, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional axis limits should be semi-open interval
4 participants