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

IJCK R2C12 #29

Closed
bryanwweber opened this issue Sep 27, 2017 · 13 comments
Closed

IJCK R2C12 #29

bryanwweber opened this issue Sep 27, 2017 · 13 comments

Comments

@bryanwweber
Copy link
Member

Is it possible to create for example two common-properties composition blocks A and B and
refer to them in the DataPoint keys?

@bryanwweber
Copy link
Member Author

Ref:

From Kyle:

Regarding two composition blocks inside common-properties, I think this would be possible, as long as field are given different YAML tags. Perhaps we should add a test for this.

@bryanwweber
Copy link
Member Author

I actually don't think this is possible, unless the blocks were named differently. That is,

common-properties:
  compositon: &compA
    - ...
  composition: &compB
    - ...

is prohibited AFAIK (or perhaps the keys would be merged, it might depend on the YAML library implementation). However, if we allow any property in the common-properties, then the two composition blocks don't have to be named composition and this would work fine.

@bryanwweber
Copy link
Member Author

See also pr-omethe-us/PyKED#67

@kyleniemeyer
Copy link
Member

I think this would work:

common-properties:
  compositionA: &compA
    - ...
  compositionB: &compB
    - ...

since the data points would have

  composition: *compA

... right?

@bryanwweber
Copy link
Member Author

But our schema would reject the keys compositionA and compositionB at the moment (I think)

@kyleniemeyer
Copy link
Member

hmm... do we look at what's in common-properties, or just led the YAML parser process the tags/links and then remove the whole block?

@bryanwweber
Copy link
Member Author

I actually don't know... I know that we have a common-properties section in the schema, though...

@bryanwweber
Copy link
Member Author

bryanwweber commented Oct 2, 2017

OK, so I did a quick test of this. We do check the specific keys that are allowed in the common-properties section, so specifying compositionA and compositionB would be rejected by the validator. We can certainly disable that check and allow multiple specifications of the same property in common-properties.

That said, I'm not sure if I like the idea of allowing multiple specifications of the same property in the common-properties section in files we put into the database. It seems like allowing this would be more likely to lead to errors (was this data point compositionA or compositionB? Which anchor refers to which again? etc.). In addition, requiring multiple files for multiple compositions (e.g., equivalence ratios) might help the files from becoming unwieldy and un-human-readable (which we keep emphasizing). If we allow multiple, e.g., equivalence ratios in one file, I can see people trying to represent hundreds of data points in a single file, which is what I'm worried about.

So my suggestion is that we enforce a cap on the number of allowed data points (and disallow using multiple specifications of a property in common-properties) in a single file that we admit to the database, and let people do whatever they want with their own internal files. I'll try to do some work on this under pr-omethe-us/PyKED#67.

Thoughts?

@kyleniemeyer
Copy link
Member

We definitely want to encourage people using a single file for a single series, so having multiple common composition fields seems to go against this. (They can still vary composition across data points, of course).

If we only allow a single version of a field to be declared in common-properties, do we need to set a cap on the number? I'd prefer not to do that.

@bryanwweber
Copy link
Member Author

Ok so you're saying not to allow multiple values for a property in common-properties? I'm fine with that. What I was suggesting above is that we remove the current technical limitation on this behavior (so people can treat their personal files how they want) but enforce one of each property for files we admit to the database (and separately cap the maximum number of data points per file).

With the current implementation, there's no need to cap the number of times a property is specified because it has to have the name we define, and multiple definitions will either merge, overwrite each other, or raise an error (I'm not sure which). However, the current implementation is not easy to extend with new properties since each one has to be specified in the common-properties section individually. So we need a new way to do the common-properties 😁

@kyleniemeyer
Copy link
Member

So are you saying remove the limitation of what can be stored in common-properties? Right now, isn't it just anything that can be used for a data point? I guess I don't see the benefit of further loosening it from that.

Also, I don't see the benefit of restricting the number of data points per file—practically this will be limited, so why bother making an artificial upper limit?

@bryanwweber
Copy link
Member Author

bryanwweber commented Oct 3, 2017

  1. Right now, only pressure, ignition-type, composition, and pressure-rise can be used in common-properties, see https://github.com/pr-omethe-us/PyKED/blob/master/pyked/schemas/chemked_schema.yaml#L25

    Which properties should be in common-properties? PyKED#67 is a proposal to remove the restriction on what can be included in the common-properties, but I don't know what's the best way to do that, and it also depends if we want to allow duplicates of the same type of information what the "best" implementation will be.

  2. OK, that seems fine

bryanwweber added a commit that referenced this issue Oct 8, 2017
Basically say "Not right now, stay tuned"
@kyleniemeyer
Copy link
Member

I think our response here is good enough.

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

No branches or pull requests

2 participants