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

Add per entry and per property meta data field. #463

Merged
merged 43 commits into from
Jun 16, 2023

Conversation

JPBergsma
Copy link
Contributor

During the discussion on ranged properties, we thought it would be a good idea to have a method for storing per entry metadata for a property. This has been discussed in Issue #410 and #462.
In this PR, I have added a brief description for this metadata field, so I can use it for the ranged properties in PR #452.

I explicitly allow metadata fields to also have metadata of its own.
When a ranged property would have uncertainty values. That property would be just as large as the original ranged property, so I think it makes sense to allow metadata fields to have metadata themselves.

We can discuss this further during the optimade meeting of next Friday.

Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

Thank you for putting together this PR. I like the idea of introducing the metadata mechanism now and specifying the actual metadata properties later.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@JPBergsma
Copy link
Contributor Author

During the last online optimade meeting, the point was raised that we should also define how the metadata fields should be defined in the property definitions.

I expect that some metadata subfields occur in several metadata properties with the same meaning. For example, many numerical properties may have a confidence interval. For now, I have specified that these should have the same $id field. (I hope that this was indeed the intention of the original $id field) but should otherwise be declared repeatedly.
Another option would be to allow custom data types that could be defined elsewhere, but I think that would be too complicated to include in this PR.

I have also added a metadata_for field to the metadata dictionary, as asked by @sauliusg. I do not think we reached a consensus on whether this field should be added, so I can easily remove it if that is what you prefer.

Do we want to return only the field name or the complete path including the field name in the metadata_for field?

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@rartino
Copy link
Contributor

rartino commented Apr 27, 2023

As far as I can see, we still don't have a solution to the concerns of some people in the last meeting that it is an issue to limit the field name namespace; i.e., we really should use a different meta name delimiter?

(@sauliusg alternatively argued to reduce the _meta suffix naming to SHOULD or MAY level, and rely completely on metadata_for in the property definition for the connection was the solution. But, I'm still reluctant to that, since because of code design convenience I want to be able to write code that can detect and parse ranged properties in the output without having to know about the property definitions. That will only be possible if the metadata field name is derivable from the field name.)

@sauliusg
Copy link
Contributor

sauliusg commented May 4, 2023

argued to reduce the _meta suffix naming to SHOULD or MAY level, and rely completely on metadata_for in the property definition for the connection was the solution. But, I'm still reluctant to that, since because of code design convenience I want to be able to write code that can detect and parse ranged properties in the output without having to know about the property definitions. That will only be possible if the metadata field name is derivable from the field name.

I would once again advise against assigning special meanings or special requirements to names. IMHO this is a technological debt, and a pretty nasty one.

Special names pollute the name space, and if we go that way and try to apply it universally, we might very soon arrive at contradictory requirements. We were bitten several times by a decision to use "special" names, do I am wary about such choices.

If a data element "is_metadata_for" is embedded into the data response, what is a problem to test it and get the necessary information during processing?

@JPBergsma JPBergsma mentioned this pull request Jun 15, 2023
@merkys
Copy link
Member

merkys commented Jun 16, 2023

Is anyone working on resolving the conflict? If not, I can give it a stab.

rartino
rartino previously approved these changes Jun 16, 2023
@rartino
Copy link
Contributor

rartino commented Jun 16, 2023

@merkys Sorry, didn't see your comment. Already did the merge - it was a bit nasty, but not too bad.

I'm happy with this PR as it now is. There is apparently some disagreement over what the key inside "meta" should be to communicate metadata for properties, property_metadata or attributes_metadata. Perhaps @giovannipizzi and @sauliusg who were part of the referenced discussion can take a side on that? Or, for that matter, anyone else.

@merkys
Copy link
Member

merkys commented Jun 16, 2023

@merkys Sorry, didn't see your comment. Already did the merge - it was a bit nasty, but not too bad.

No problem, I wanted to get an ACK before embarking on it anyway.

optimade.rst Outdated Show resolved Hide resolved
@merkys merkys requested a review from rartino June 16, 2023 07:24
merkys
merkys previously approved these changes Jun 16, 2023
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

I re-approve the proposal. I prefer the current key name property_metadata.

@merkys merkys requested review from vaitkus and ml-evs June 16, 2023 07:25
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
Co-authored-by: Antanas Vaitkus <antanas.vaitkus90@gmail.com>
@merkys merkys requested a review from vaitkus June 16, 2023 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding per property meta_data field.
6 participants