Skip to content

Remove NXcontainer duplicates#566

Closed
mtwharmby wants to merge 2 commits intonexusformat:masterfrom
mtwharmby:RemoveNXcontainerDuplicates
Closed

Remove NXcontainer duplicates#566
mtwharmby wants to merge 2 commits intonexusformat:masterfrom
mtwharmby:RemoveNXcontainerDuplicates

Conversation

@mtwharmby
Copy link
Copy Markdown
Contributor

This PR removes fields which were moved over into NXsample and are therefore no longer necessary in the NXcontainer class. This was discussed (again, some time ago!) with @markbasham

Signed-off-by: Michael Wharmby michael.wharmby@diamond.ac.uk

Michael Wharmby added 2 commits April 21, 2017 14:14
Duplicated fields now removed. beam, shape and orientation are still 
required in case the reference container measurement was made with a 
different geometry/energy. The remaining fields allow us to show the 
hierarchy of samples/containers.

Signed-off-by: Michael Wharmby <michael.wharmby@diamond.ac.uk>
Signed-off-by: Michael Wharmby <michael.wharmby@diamond.ac.uk>
@zjttoefs
Copy link
Copy Markdown
Contributor

I don't understand. What are the new fields in NXsample?

@mtwharmby
Copy link
Copy Markdown
Contributor Author

I reviewed NXcontainer with @markbasham a couple of months back and realised that density, chemical_formula, chemical_formula_weight (relative_molecular_mass) had either been added to NXsample since I wrote NXcontainer or were already present. Also, @markbasham and I discussed using an NXsample class to store these data for a container as this is/was what the NIAC had decided was a better method for storing these data.

The only problem I can see for a Pair Distribution Function experiment usecase is there will be no packing_fraction for the sample, which is a necessary field. I'll discuss this offline with @markbasham

@zjttoefs
Copy link
Copy Markdown
Contributor

zjttoefs commented Apr 24, 2017

But doesn't NXcontainer describe the container and NXsample the sample? These two different things can have different chemical formulae and densities. No? Was that not the point for having NXcontainer?

@stuartcampbell
Copy link
Copy Markdown
Member

@zjttoefs yes i think you are correct and i agree the container material could be something that is useful to know.

@mtwharmby
Copy link
Copy Markdown
Contributor Author

That was the original idea, but I was given the impression that as a measurement of a container needs very similar information storing as an NXsample, we should be using NXsample. And as we need a few extra fields (e.g. some way of showing the relationship between multiple containers) we still need to NXcontainer class.

If I should be using NXcontainer to describe container only measurements, I will need to add a few new fields and it runs the risk of duplicating a lot of what is already in NXsample/NXsample_component. I don't mind doing this, but it'll be a different pull request!

@zjttoefs
Copy link
Copy Markdown
Contributor

Agree, ignoring aspects like geometry and shape NXcontainer is just a special case of NXsample_component. Those are important aspects, though and they usually mean that you can measure the container on its own, but often not the sample (under the same conditions). Especially when you have an involved protocol of measurements to identify separate contributions to the data I see value in NXcontainer.
We ran out to time to approve that in the last NIAC meeting as far as I can tell from the minutes and remember. There was no official line against it.

@zjttoefs
Copy link
Copy Markdown
Contributor

Either way it would be good to see a full working example of how you see that used, with or without these fields. If we don't have that, it could have been the reason we didn't discuss that in detail at the NIAC.
For the time being: Are you interested in keeping this pull request open?

@mtwharmby
Copy link
Copy Markdown
Contributor Author

I'll close it for now. I have an offline meeting with @markbasham in a couple of weeks to discuss this sort of thing.

@mtwharmby mtwharmby closed this Apr 27, 2017
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.

3 participants