mantle-convection-constrained / terratools

Tools to read, analyse and visualise models written by the TERRA mantle convection code
https://terratools.readthedocs.io/en/latest/
MIT License
7 stars 5 forks source link

Add arbitrary fields #147

Closed jamespanton93 closed 8 months ago

jamespanton93 commented 8 months ago

Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable.

This PR addresses #146 to allow users to add a new field to the TerraModel with an arbitrary name.

Before your first pull request:

For all pull requests:

For new features/models or changes of existing features:

anowacki commented 8 months ago

This is an interesting change. terratools was initially set up deliberately only to hold certain fields. Other fields—combinations of them or new derived ones—could be managed as just normal NumPy arrays outside of the TerraModel instance. But I appreciate that the convenience of plotting these and otherwise analysing them using terratools's machinery is very nice, so if users want this then we should accommodate it—so thanks for the PR and merge.

Perhaps we should just ditch the label thing and remove the limits on what you can call a field? Then name is just the name of the field and a field's name defines what it should be like and how it should behave.

(The reason I originally limited what fields could be called was because it seemed inevitable that different people would call the same field different things. Then different people's code would become incompatible because someone's "t" didn't match with someone else's "T", or someone else's "temperature". Perhaps strong, documented conventions on what fields should be called for certain things is the way to go instead?)

jamespanton93 commented 8 months ago

I think you're right about creating documented conventions on what fields should be called. I should think the issue of un-matched field names would be limited to derived and combination fields, as users are unlikely to be applying a new temperature / composition / velocity field to their TerraModel, but the convention should be documented anyway.

About the label thing, the benefit I see with it is that it encourages users to provide a bit of context to the field that they are adding. If we decide to do away with it we would have to account for this in the plotting the functions.

anowacki commented 8 months ago

Ah, I see what the label is for. Yes, it's nice to have the option of giving a description of the field. It's just that in the code I see that you currently allow an arbitrary name only when the label is set, which confused me.

I see that the global (meant to be constant) dicts which hold the predetermined field names are getting changed when you add an arbitrary field. I'm not sure that's really what we want to do. I think I'd rather just get rid of the restriction on the name entirely, and then these global dicts are only for reading or creating fields which have a particular convention in terms of number of components that it's convenient to expose. This is backward-compatible, which is nice.