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

feat: track gain and mdoc files alongside runs #280

Merged
merged 13 commits into from
Oct 4, 2024

Conversation

jgadling
Copy link
Contributor

This is a draft to make sure we're aligned on the structure (and naming!) for keeping track of mdoc and gains files associated with a run. Once we agree on names/structures, I'll update the API with the changes.

Also feel free to just add commits to this branch if that's easier.

Copy link

@anchi2c anchi2c left a comment

Choose a reason for hiding this comment

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

What happened to per_section_parameters ? When I look at the changes, they are removed in the new changes.

@jgadling
Copy link
Contributor Author

What happened to per_section_parameters ? When I look at the changes, they are removed in the new changes.

We're going to remove them for now because they'll be coming in a later update. It wouldn't hurt to just include them early, except that I'm worried we'll be making breaking changes (like removing/renaming fields in these tables) once we start working with that data, so in that case it would be better to add them in once we're more confident that the structure of those tables is stable.

apiv2/platformics/graphql_api/core/deps.py Outdated Show resolved Hide resolved
apiv2/schema/schema.yaml Show resolved Hide resolved
apiv2/schema/schema.yaml Show resolved Hide resolved
maximum_value: 180
unit:
symbol: °
descriptive_name: degrees
PerSectionAlignmentParameters:
name: PerSectionAlignmentParameters
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The in_plane_rotation field will be a collection of 4 float values. Should we make it a vector or a string?
  • The beam_tilt field is not going to be supported for now.
  • We should add support for the is_canonical boolean field and the float volume_x_rotation.

Copy link
Contributor Author

@jgadling jgadling Sep 27, 2024

Choose a reason for hiding this comment

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

Supporting vectors and other strongly typed json fields is pretty doable in the API - we already support 2d arays of floats, so supporting 1d arrays is easy to add. I haven't tried to add complex types to the client before though, so that could get interesting. Do you think in_plane_rotation would be best represented as a list of floats?

Copy link
Contributor

Choose a reason for hiding this comment

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

@uermel Could we have your thoughts on what is the best way to represent in_plane_rotation for the end user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decision: This will be a 2d rotation matrix, basically list[list[float]]

@jgadling jgadling merged commit 483eee8 into main Oct 4, 2024
7 checks passed
@jgadling jgadling deleted the jgadling/track-extra-files branch October 4, 2024 16:20
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.

3 participants