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

bfopen: store both the global and series metadata in the returned array #825

Merged
merged 2 commits into from
Dec 15, 2013

Conversation

melissalinkert
Copy link
Member

@sbesson
Copy link
Member

sbesson commented Dec 11, 2013

Tested using some Metamorph data where both getGlobalMetadat() and getSeriesMetadata() return non empty hash tables. Commit above fails because MetadataTools.merge does not return any output. A fix for the bfopen.m function can be found and cherry-picked at sbesson@b2c2187.

Note this PR may introduce a breaking change with the Global prefix, i.e. to retrieve the DateTime value

data{1,2}.get('DateTime')

would return null and have to be substituted by:

data{1,2}.get('Global DateTime')

That being said, in the current implementation of bfopen, the global metadata is already missing and I can't tell how many users are really retrieving the original metadata from MATLAB. A third way would be to allow this prefix to be set in the function input. In all cases, we may want to open a doc PR to make this change explicit. Thoughts?

@sbesson
Copy link
Member

sbesson commented Dec 11, 2013

To summarise, we have four ways to move forward:

  • merge getGlobalMetadata() prefixed with Global and unprefixed getSeriesMetadata()
  • merge getSeriesMetadata() prefixed with the series name and unprefixed getGlobalMetadata() (similarly to the output of getMetadata())
  • merge getSeriesMetadata() prefixed with the series name and getGlobalMetadata() prefixed with Global
  • merge unprefixed getSeriesMetadata() and getGlobalMetadata()

Having looked more into the relevant code, I now realise that 6055791 that was introduced in dev_4_4 was a breaking change already :) Once we decide on a final implementation, happy to open a PR to clarify the relevant documentation page.

@melissalinkert
Copy link
Member Author

New commit pushed - I didn't cherry-pick sbesson@b2c2187 as it doesn't appear to store merged Map into result.

My reasoning for merging global into series so that global metadata has the Global prefix was that it won't break anyone currently retrieving original metadata - the series-specific keys will remain the same. Prefixing series keys or merging with no key prefixes would cause the series-specific keys and/or values to be different.

Technically the keys aren't public API, though, so if you think that will cause a problem then it can be changed.

@sbesson
Copy link
Member

sbesson commented Dec 15, 2013

BIOFORMATS-merge-develop has been passing with the last commit. Tested using Metamorph data, both global and series metadata are correctly stored in the output cell array per series. Ready to merge.

melissalinkert added a commit that referenced this pull request Dec 15, 2013
bfopen: store both the global and series metadata in the returned array
@melissalinkert melissalinkert merged commit 5734bf3 into ome:develop Dec 15, 2013
@melissalinkert
Copy link
Member Author

--no-rebase

@melissalinkert melissalinkert deleted the matlab-metadata branch September 26, 2014 13:31
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 this pull request may close these issues.

None yet

2 participants