nmrML / nmrCV

Development of nmrCV terminology formerly included in the nmrML repository.
0 stars 0 forks source link

Rework NMR instrument manufacturer #12

Open NRayya opened 5 months ago

NRayya commented 5 months ago

We decided that for the time being, we will consider both manufacturers of whole spectrometers and parts as "device manufactures".

NRayya commented 5 months ago

We decided that for the time being, we will consider both manufacturers of whole spectrometers and parts as "device manufactures". better to use OBI:device, which already contains subclasses for parts of NMR instruments (console, magnet) and OBI:NMR instrument under measurement device

StroemPhi commented 2 weeks ago

@NRayya should I interpret your last comment that part of resolving this issue is not just reusing OBI:manufacturer and subsume the above listed NMR related ones under there, but also to do the same for the instruments and instrument parts under OBI:device and OBI:NMR instrument repsectively? If yes, we should add this to the description of the issue.

NRayya commented 1 week ago

@NRayya should I interpret your last comment that part of resolving this issue is not just reusing OBI:manufacturer and subsume the above listed NMR related ones under there, but also to do the same for the instruments and instrument parts under OBI:device and OBI:NMR instrument repsectively? If yes, we should add this to the description of the issue.

Done

StroemPhi commented 1 day ago

While working on the implementation in https://github.com/nmrML/nmrCV/tree/12-rework-NMR-instrument-manufacturer I noticed that we have a class 'NMR instrument vendor' which is defined as an 'instrument attribute' (basically a piece of information) that seems redundant to me, since we already have 'NMR device manufacturer'. Can we deprecate this as part of this issue?

StroemPhi commented 1 day ago

Working on subsuming existing NMR devices under the respective OBI:device branch entails having to deprecate nmrCV classes that were already defined in OBI (coming originally from the nmrCV crew i.e. Daniel Schober). Now, in the current nmrCV some of these NMR devices are grouped under NMRCV:cardinal part of NMR instrument, but as this class is being dropped when the OBI equivalents are used instead, its semantics gets lost. The solution would be to assert on its children in OBI that they are part of an OBI:NMR instrument which is done in OBI only for OBI:NMR sample tube, OBI:NMR probe and OBI:magic angle spinning rotor. I wil lmake these asserions for now in nmrCV, but since this is considered axiom injection, we will need to get this into OBI in a separate issue. So this is an interim fix, to get this issue resolved. When these axioms are requested by us in OBI we should also use this opporunity to revise the definitions of these classes, as they are not in line with the OBO Foundry best practices.

NRayya commented 21 hours ago

While working on the implementation in https://github.com/nmrML/nmrCV/tree/12-rework-NMR-instrument-manufacturer I noticed that we have a class 'NMR instrument vendor' which is defined as an 'instrument attribute' (basically a piece of information) that seems redundant to me, since we already have 'NMR device manufacturer'. Can we deprecate this as part of this issue?

We have now a separate issue for vendors: https://github.com/nmrML/nmrCV/issues/31

NRayya commented 21 hours ago

Working on subsuming existing NMR devices under the respective OBI:device branch entails having to deprecate nmrCV classes that were already defined in OBI (coming originally from the nmrCV crew i.e. Daniel Schober). Now, in the current nmrCV some of these NMR devices are grouped under NMRCV:cardinal part of NMR instrument, but as this class is being dropped when the OBI equivalents are used instead, its semantics gets lost. The solution would be to assert on its children in OBI that they are part of an OBI:NMR instrument which is done in OBI only for OBI:NMR sample tube, OBI:NMR probe and OBI:magic angle spinning rotor. I wil lmake these asserions for now in nmrCV, but since this is considered axiom injection, we will need to get this into OBI in a separate issue. So this is an interim fix, to get this issue resolved. When these axioms are requested by us in OBI we should also use this opporunity to revise the definitions of these classes, as they are not in line with the OBO Foundry best practices.

It was decided to do the fixes as needed in nmrCV and contact OBI team so that they import our terms and not the other way around as nmrCV is the NMR specific ontology. This will also solve the axiom injection