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

Boundary for SpatialSeries #567

Merged
merged 16 commits into from
Mar 25, 2024
Merged

Boundary for SpatialSeries #567

merged 16 commits into from
Mar 25, 2024

Conversation

mavaylon1
Copy link
Contributor

@mavaylon1 mavaylon1 commented Jan 19, 2024

Summary of changes

Optional attribute, bounds, for SpatialSeries data.

Fix #524

Checklist

For all schema changes:

  • Add release notes for the PR to docs/format/source/format_release_notes.rst.

If this is the first schema change after a schema release (i.e., the version string in core/nwb.namespace.yaml does not
end in "-alpha"), then:

  • Update the version string in core/nwb.namespace.yaml and core/nwb.file.yaml to the next major/minor/patch
    version with the suffix "-alpha". For example, if the current version is 2.4.0 and this is a minor change, then the
    new version string should be "2.5.0-alpha".
  • Update the value of the version variable in docs/format/source/conf.py to the next version without the
    suffix "-alpha", e.g., "2.5.0".
  • Update the value of the release variable in docs/format/source/conf.py to the next version with the suffix
    "-alpha", e.g., "2.5.0-alpha".
  • Add a new section in the release notes docs/format/source/format_release_notes.rst for the new version
    with the date "Upcoming" in parentheses.

@mavaylon1
Copy link
Contributor Author

mavaylon1 commented Jan 19, 2024

Fix #524

@mavaylon1 mavaylon1 marked this pull request as ready for review January 19, 2024 16:55
@mavaylon1
Copy link
Contributor Author

mavaylon1 commented Jan 19, 2024

Review Notes: For the dtype my assumption was to use float32 (per the original issue poster's notes). When I think of a boundary, I imagined this: [.56, 1.4]. I don't believe I need to do anything special to support the fact it would be in an list.

@mavaylon1 mavaylon1 requested a review from oruebel January 19, 2024 16:57
core/nwb.behavior.yaml Outdated Show resolved Hide resolved
core/nwb.behavior.yaml Outdated Show resolved Hide resolved
@rly
Copy link
Contributor

rly commented Mar 5, 2024

Pinging @mavaylon1 about comments above.

@rly
Copy link
Contributor

rly commented Mar 5, 2024

Fix #524

FYI @mavaylon1 GitHub will only automatically close this issue if the "fix X" syntax is used in the PR description, not a comment, so I recommend that you edit the first post to include that. (Example where the PR comment did not close the issue)

@rly
Copy link
Contributor

rly commented Mar 5, 2024

2.7.0 has been released, so this will have to move to 2.8.0. Please adjust the version strings appropriately as described in the checklist in the first post.

@mavaylon1
Copy link
Contributor Author

2.7.0 has been released, so this will have to move to 2.8.0. Please adjust the version strings appropriately as described in the checklist in the first post.

2.7.0 has not been released

@rly
Copy link
Contributor

rly commented Mar 5, 2024

2.7.0 has been released, so this will have to move to 2.8.0. Please adjust the version strings appropriately as described in the checklist in the first post.

2.7.0 has not been released

I released it last month while you were away https://github.com/NeurodataWithoutBorders/nwb-schema/releases/tag/2.7.0 to try to get a pynwb release that includes support for it, but I did not manage to get the pynwb side done in time before my vacation. So PyNWB and MatNWB are due for a release that adds 2.7.0 support.

@mavaylon1 mavaylon1 requested a review from rly March 6, 2024 19:24
core/nwb.behavior.yaml Outdated Show resolved Hide resolved
@rly
Copy link
Contributor

rly commented Mar 6, 2024

Can you revert the hdmf-common-schema update for reasons mentioned here: hdmf-dev/hdmf-common-schema#80 (comment) ?

@mavaylon1
Copy link
Contributor Author

Can you revert the hdmf-common-schema update for reasons mentioned here: hdmf-dev/hdmf-common-schema#80 (comment) ?

Done. I also added this to the PR checklist.

@rly
Copy link
Contributor

rly commented Mar 8, 2024

Looks good. Thanks! Out of an abundance of caution, let's not merge this until we implement and test support for this change in a branch/PR of PyNWB. Can you do that @mavaylon1 ?

@mavaylon1
Copy link
Contributor Author

Looks good. Thanks! Out of an abundance of caution, let's not merge this until we implement and test support for this change in a branch/PR of PyNWB. Can you do that @mavaylon1 ?

Will do.

@mavaylon1
Copy link
Contributor Author

@rly I made a test for this but it's getting weird on pynwb where we are on a branch built from your branch for schema 2.7 in pynwb branch that has nwb-schema pointing to a branch.

Let's merge this. Especially since our new policy is to point our schemas to the latest release and not the main/dev of the schema repo, meaning this won't mess with pynwb.

@mavaylon1 mavaylon1 merged commit 942672b into dev Mar 25, 2024
5 checks passed
@mavaylon1 mavaylon1 deleted the spatial branch March 25, 2024 23:02
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.

Request: metadata field in SpatialSeries for boundary range
3 participants