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

New version of PASTA's .eln file #70

Merged

Conversation

SteffenBrinckmann
Copy link
Collaborator

@nicobrandt
Copy link
Contributor

A few remarks while manually checking the JSON-LD:

  • Nested entries like sdPublisher, author, etc. should ideally be flattened and only referenced via their @id.
  • Some entries like the author or the flexible metadata are missing a @type.
  • Side note: Have we discussed yet whether variableMeasured is the best place for the flexible metadata beyond the initial suggestion?

@FlorianRhiem
Copy link
Contributor

SampleDB currently fails to import the .eln file as the file ./temporary_pastaTest/StandardOperatingProcedures/SEM.md is referenced, however it does not exist. The temporary_pastaTest directory only contains a file data_hierarchy.json, nothing else.

Aside from that, there seem to be a lot of additional properties there, which I'm not sure whether they are useful to display to a user, e.g. _attachments.v0.json.digest to _attachments.v51.json.digest (and the respetive lengths, revpos, etc). These are also listed for File objects, not just for Dataset objects, which isn't valid.

The example directory README should be updated to reflect the new ro-crate-metadata.json, right?

@nicobrandt I don't think so, but going by schema.org, a Dataset can contain a PropertyValue either as variableMeasured or variablesMeasured, aside from it being used as identifier. variablesMeasured is retired and recommends using variableMeasured, so unless we want to deviate from the existing properties, variableMeasured is the most fitting.

@jmanideep
Copy link
Contributor

jmanideep commented May 15, 2024

A few remarks from my side:

  • As per specification, the folder name should match the name of the archive.
  • According to the discussion in #9, the hasPart property in the second object in the @graph, should have only the relative paths of the folders. It should be updated in the specification too.
  • Additionally, some folders, for eg: { "@id": "./temporary_pastaTest/Steel/" }, { "@id": "./temporary_pastaTest/SurfaceEvolutionInTribology/" }, are listed in the hasPart but are not present in the archive.
  • Furthermore, some of the files referenced in the JSON-LD are missing in the archive.

@NicolasCARPi
Copy link
Contributor

@SteffenBrinckmann, a few remarks:

  • you should run the eln2md script so the PR includes changes to the ro-crate-metadata.json in the diff, and it becomes easier to point things
  • instead of using ORCID in the @author node, you should use identifier:
"identifier": "https://orcid.org/0000-0003-3432-2297"
  • what is the shasum property? The hashing function name must be mentioned.
  • regarding the signature, it's great, I could verify it 👍

@SteffenBrinckmann
Copy link
Collaborator Author

@nicobrandt @jmanideep @FlorianRhiem @NicolasCARPi Thank you all for the feedback: helpful and I will incorporate it in the next version.
We have not agreed an all of the "methods" used here and the idea here is not to fix anything. It is meant as a test area for ideas and see if/how they could work. Trying something in code is - in my view - better than theoretical discussions. Once we finally agree on things, then we can write tests for them and ... . Nevertheless, this is just the output of code and that code is easily changed, no hard feelings if we agree on alternative methods.

@SteffenBrinckmann
Copy link
Collaborator Author

OK, this is an improved version that addresses all mentioned issues but one:

  • @jmanideep "According to the discussion in Rules for metadata.json nodes declaration #9, the hasPart property in the second object in the @graph, should have only the relative paths of the folders. It should be updated in the specification too."
    I do not understand: does that mean that './' cannot contain data?

Obviously from the failed github action, the tooling is also broken: I have some fixes but I did not want to mingle them into this.

Copy link
Contributor

@NicolasCARPi NicolasCARPi left a comment

Choose a reason for hiding this comment

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

Can you run the README generating script?

@SteffenBrinckmann
Copy link
Collaborator Author

@NicolasCARPi Sorry my bad

@SteffenBrinckmann SteffenBrinckmann marked this pull request as draft October 6, 2024 08:06
@NicolasCARPi
Copy link
Contributor

So here 002_DataFiles/ appears as part of PastasExampleProject/. But is not described as part of ./.

Not saying it's wrong, but I'd need to fix my import logic to handle this case, because currently it won't work as expected and these files will be ignored.

@SteffenBrinckmann SteffenBrinckmann marked this pull request as ready for review October 7, 2024 06:32
@SteffenBrinckmann SteffenBrinckmann marked this pull request as draft October 7, 2024 06:33
@SteffenBrinckmann
Copy link
Collaborator Author

@NicolasCARPi I will look into that

@jmanideep
Copy link
Contributor

@SteffenBrinckmann In the previous version of your example (here), the hasPart property in the second object(where "@id": "./") in the @graph has the following structure:

"hasPart": [ { "@id": "./temporary_pastaTest/Steel/" }, { "@id": "./temporary_pastaTest/SurfaceEvolutionInTribology/" }, { "@id": "./temporary_pastaTest/IntermetalsAtInterfaces/" }, { "@id": "./temporary_pastaTest/data_hierarchy.json" } ],.

However, according to specification, the hasPart property in this node should contain only relative paths of the folders. This means that entries like { "@id": "./temporary_pastaTest/data_hierarchy.json" }, should not be included here. Instead, such files should be listed in the hasPart of the respective Dateset node.

@SteffenBrinckmann SteffenBrinckmann marked this pull request as ready for review October 8, 2024 12:36
@SteffenBrinckmann
Copy link
Collaborator Author

Should now fit

@SteffenBrinckmann SteffenBrinckmann merged commit b00b2da into TheELNConsortium:master Oct 10, 2024
1 check passed
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.

5 participants