search

FaradayInstitution / BPX

BPX schema in pydantic and JSON schema format, and parsers
MIT License
25 stars 12 forks source link

Issue 26 user defined params #44

Closed rtimms closed 11 months ago

rtimms commented 1 year ago

Allows user-defined parameters to be added using the field ["Parameterisation"]["User-defined"].

Fixes #26

ejfdickinson commented 1 year ago

Do we want the string alias to be User-defined verbatim, with hyphen and lowercase "defined"? I think that's most consistent with the usage elsewhere in the schema.

rtimms commented 1 year ago

Apologies, my description is inconsistent with the code I wrote! In the code, the alias is "User defined". I've updated the description to match the code.

EDIT: I also need to add the hyphen. Will update.

ejfdickinson commented 1 year ago

Yeah, I think the hyphen is required - it's pedantic to say so but that is kind of the point!

ejfdickinson commented 1 year ago

@rtimms One issue that occurred to me that I haven't been able to test, but perhaps you could.

I wasn't sure from your latest type-fixing commit - do we allow an arbitrary nested structure in the JSON under "User-defined". It feels like we should - that is, we shouldn't care if there is extra JSON hierarchy within this zone.

Would treating all dict as InterpolatedTable break this?

rtimms commented 1 year ago

We do not currently allow this kind of structure. Everything in "User-defined" must be of the type FloatFunctionTable. I'm reaching the limit of my pydantic knowledge now, but what you are suggesting should be possible. I'll give it a try, but may need to call in reinforcements!

ejfdickinson commented 1 year ago

@rtimms Happy to save that for a future update if necessary. Besides anything else, we would need to more carefully define what is and is not an InterpolatedTable.

rtimms commented 11 months ago

@ejfdickinson, let's leave the careful definition of what is and is not an InterpolatedTable for a future issue. Otherwise, does this PR look ok to you?

ejfdickinson commented 11 months ago

@rtimms Yes, if the tests are passing including the example I generated, that's fine. Can you make a new issue for supporting arbitrary JSON hierarchy under User-defined?

rtimms commented 11 months ago

Thanks, new issue is here https://github.com/FaradayInstitution/BPX/issues/46

Can you hit "approve" so this can be merged?