Application definitions #63

SimonHeybrock · 2022-10-17T11:20:44Z

Experimental (and incomplete) support for reading NXcanSAS.

Also fixes #65.

jl-wynen · 2022-10-18T08:19:47Z

src/scippnexus/definitions/nxcansas.py

+        self._group = group
+
+    def child_strategy(self, group: H5Group):
+        if (definition_class := group.attrs.get(self.class_attribute)) is not None:


This requires child classes to define class_attribute and strategies. @nvaytet and I talked about this recently and concluded that we should avoid this kind of requirement.
Can we avoid it here? That is, can this be implemented by defining those fields in ApplicationDefinitionStrategy? Or can we use a decorator instead of inheritance?

Sure, this file is just a quick draft, to see if the strategy mechanism can be used for this. There are many things to improve here.

jl-wynen · 2022-10-18T08:22:30Z

src/scippnexus/definitions/nxcansas.py

+
+    def child_strategy(self, group: H5Group):
+        if (definition_class := group.attrs.get(self.class_attribute)) is not None:
+            return self.strategies.get(definition_class)


This function returns None when

there is no class attribute in the file

there is no child strategy for the given attribute

Shouldn't the 2nd case be a hard failure, i.e. something is not supported by scippnexus?

I think that is for the application definition to decide. Without a custom strategy, child strategies are always set to None, meaning the default strategy will be used by the child. Conceptually, this means the parent "does not care about or control" the child.

jl-wynen · 2022-10-18T08:25:16Z

src/scippnexus/definitions/nxcansas.py

+    pass
+
+
+NXcanSAS.classes = [SASroot, SASentry, SASdata, SAStransmission_spectrum]


By inheriting from NXcanSAS in all classes, this application definition supports arbitrary nesting. E.g. a root inside data. Does this make sense? Should we limit it more? For instance using something like the nexus loader prototype in ess?

I had considered this. My conclusion was that in many cases structure may not be something people want to enforce (it will prevent reading partially broken/non-standard files, for example). At the same time, you can write your strategy in a way that it enforces structure.

jl-wynen · 2022-10-18T08:29:38Z

src/scippnexus/nxdata.py

@@ -2,7 +2,7 @@
 # Copyright (c) 2022 Scipp contributors (https://github.com/scipp)


This approach seems quite invasive in that it requires the NX* classes to know about and handle strategies. Is this necessary or could it be implemented on a higher level like the loader prototype in ess?
So what kind of modification to the behaviour of, e.g. NXdata does the application definition need to make which cannot be done as post processing?

This approach seems quite invasive in that it requires the NX* classes to know about and handle strategies.

Well, this is the strategy pattern. It does not have to know about the details of the strategy.

Is this necessary or could it be implemented on a higher level like the loader prototype in ess?

Not really, you would need to fall back to h5py, since scippnexus will generally not be able to load groups that follow an application definition, e.g., because they define axes (dims) in a custom manner.

So what kind of modification to the behaviour of, e.g. NXdata does the application definition need to make which cannot be done as post processing?

Probably some could be made. But the post processing would need to reopen the file and read custom attributes, etc. So basically you could just go back and implemented your loader with plain h5py instead of scippnexus, as there might be little benefit.

jl-wynen · 2022-10-18T08:30:35Z

Overall comment to the approach in #63 (comment)

SimonHeybrock · 2022-10-24T07:28:34Z

@jl-wynen I refactored to use decorators (I will move this to a separate file once we are happy with it). One thing that bothers me is the creation of instances of the strategies for each group. I have considering changing the self argument of, e.g., NXdataStrategy or SASdata to group, and remove the __init__s that store the group. Then, instead of instantiating in the Nexus class, we would just call self._strategy.dims(self) instead of self._strategy.dims. This also looks a bit odd, but the strategy implementations themselves would be cleaner. Note they always have self._group.bla, with _group defined invisibly in a base class via the decorator.

SimonHeybrock · 2022-10-25T08:54:32Z

@jl-wynen I refactored to use decorators (I will move this to a separate file once we are happy with it). One thing that bothers me is the creation of instances of the strategies for each group. I have considering changing the self argument of, e.g., NXdataStrategy or SASdata to group, and remove the __init__s that store the group. Then, instead of instantiating in the Nexus class, we would just call self._strategy.dims(self) instead of self._strategy.dims. This also looks a bit odd, but the strategy implementations themselves would be cleaner. Note they always have self._group.bla, with _group defined invisibly in a base class via the decorator.

I made this change, since it will make support for writing more convenient. Not perfectly happy with the result, but not worse that before either.

SimonHeybrock · 2022-10-25T12:25:16Z

@jl-wynen I managed to cleanup the mechanism significantly. Can you have a look?

jl-wynen · 2022-10-28T07:27:42Z

src/scippnexus/definitions/nxcansas.py

+    def __init__(self, data):
+        self.data = data
+
+    def __write_to_nexus_group__(self, group: NXobject):


I don't think there is a need for a dunder method since we are not going to support writing wildly different objects from different libraries. A protocol like this should be sufficient:

class NXWritable(Protocol): def write_to_nexus_group(self, group:NXobject): ...

The reason for not making this a normal method was that users may want to make their existing classes "writable", without implementing a new class, and without changing the publicly visible interface.

The dunder method is public. In my mind, a dunder method relates to a general convention across libraries / codebases. In this case here, we basically have duck typing, we just require a certain method.

Yes public, but it does not interfere with tab completion, etc.

jl-wynen · 2022-10-28T07:32:06Z

src/scippnexus/definitions/nxcansas.py

+            # Note that there is also an "uncertainties" attribute. It is not clear
+            # to me what the difference is.
+            coord.attrs['resolutions'] = 'Qdev'
+            group.create_field('Qdev', sc.stddevs(da.coords['Q']))


'resolution' and 'uncertainty' seem to be different concepts. I would interpret it as resolution referring to systematic effects (e.g. binning in Q, potentially implicitly induced by the detector resolution) and uncertainty to statistical errors.

Yes, if someone actually uses both they will need to provide this in a different manner.

But shouldn't we write uncertainties instead of resolutions?

Usually people talk about "Q-resolution", and I do not think it is statistical. I don't know if anyone has decided how it will be stored, and for now just assumed they might use Scipp's variances.

Ok, if you think this is safe. I would be worried that people treat a systematic uncertainty as though it was statistical and get bad fits.

We will definitely need to check with the scientists, for now a lot of this is incomplete anyway.

jl-wynen · 2022-10-28T07:34:47Z

src/scippnexus/definitions/nxcansas.py

+        if isinstance(axes, str):
+            axes = axes.split(" ")
+        if axes is None:
+            axes = group.attrs.get('I_axes')


Can this be a string with multiple 'entries'? I.e. does this if need to be swapped with the previous one?

The documentation does not say. Will probably need to fix this in the future if it causes problems.

jl-wynen · 2022-10-28T07:35:40Z

src/scippnexus/definitions/nxcansas.py

+            axes = axes.split(" ")
+        if axes is None:
+            axes = group.attrs.get('I_axes')
+        if not isinstance(axes, list):


Can axes be a numpy array?

👍 Checking for int now instead.

jl-wynen · 2022-10-28T07:39:01Z

src/scippnexus/definitions/nxcansas.py

+class SASdataStrategy:
+
+    @staticmethod
+    def dims(group: NXobject) -> Tuple[str]:


Suggested change

def dims(group: NXobject) -> Tuple[str]:

def dims(group: NXobject) -> Tuple[str, ...]:

src/scippnexus/nxobject.py

jl-wynen · 2022-10-28T07:45:18Z

src/scippnexus/nxobject.py

        self.child_params = {}
+        self._definition = definition
+        self._strategy = self._default_strategy()


Suggested change

self._strategy = self._default_strategy()

src/scippnexus/nxdata.py

src/scippnexus/definitions/nxcansas.py

tests/application_definition_test.py

jl-wynen · 2022-11-03T12:21:39Z

src/scippnexus/definitions/nxcansas.py

        self.data = data
+        assert Q_variances in (None, 'uncertainties', 'resolutions')


I learned a surprising thing recently, assertions are removed when making optimised builds (e.g. in a wheel). So this should be a proper if-raise construct.

Didn't know that either!

SimonHeybrock added 8 commits October 17, 2022 13:20

Prototype application definition support

401a1f3

Try to get a couple of things working

4cf2cf3

Minor refactor

951e5d7

Refactor for less convoluted error name mechanism

25d93a4

Refactor to move customization to strategies

ec3afc0

Move code

0a0527d

Refactor definitions to use strategies

18b58d3

Support signal_Errors from strategy

2cb26ed

jl-wynen reviewed Oct 18, 2022

View reviewed changes

Refactor to decorators

4fef629

Cleanup

12abbf1

SimonHeybrock mentioned this pull request Oct 24, 2022

Writing NeXus files based on application definitions #65

Closed

Refactor so strategies are not instantiated with group

9e29bd4

SimonHeybrock added 7 commits October 25, 2022 12:43

Experiemnt with write support

d65247c

Cleanup, and write Q resolution

aadcf88

Split definition from strategy

b4fcdec

Refactor NXcanSAS

6fda92b

Fix how we set variances

fc949e7

Split strategies from classes for writing application definition groups

6a91ca0

Add some type hints

9cc1b78

SimonHeybrock marked this pull request as ready for review October 25, 2022 12:24

jl-wynen reviewed Oct 28, 2022

View reviewed changes

SimonHeybrock added 4 commits October 28, 2022 10:26

Cleanup

aa377c2

Rename param for clarity

c3cfae2

Remove unused code

c35146d

Write NeXus-compliant SASdata

d5e05af

SimonHeybrock added 2 commits November 2, 2022 11:38

Avoid hard-coding interpretation of Q-errors

704473b

Use better param name

831f066

jl-wynen reviewed Nov 3, 2022

View reviewed changes

Change assert to try/except

9fb10e7

jl-wynen approved these changes Nov 3, 2022

View reviewed changes

SimonHeybrock merged commit 840c260 into main Nov 3, 2022

SimonHeybrock deleted the application-definitions branch November 3, 2022 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Application definitions #63

Application definitions #63

SimonHeybrock commented Oct 17, 2022 •

edited

jl-wynen Oct 18, 2022

SimonHeybrock Oct 18, 2022

jl-wynen Oct 18, 2022

SimonHeybrock Oct 18, 2022

jl-wynen Oct 18, 2022

SimonHeybrock Oct 18, 2022

jl-wynen Oct 18, 2022

SimonHeybrock Oct 18, 2022

jl-wynen commented Oct 18, 2022

SimonHeybrock commented Oct 24, 2022 •

edited

SimonHeybrock commented Oct 25, 2022

SimonHeybrock commented Oct 25, 2022

jl-wynen Oct 28, 2022

SimonHeybrock Oct 28, 2022

jl-wynen Oct 28, 2022

SimonHeybrock Oct 28, 2022

jl-wynen Oct 28, 2022

SimonHeybrock Oct 28, 2022

jl-wynen Oct 28, 2022

SimonHeybrock Oct 28, 2022

jl-wynen Oct 28, 2022

SimonHeybrock Oct 28, 2022

jl-wynen Oct 28, 2022

SimonHeybrock Oct 28, 2022

jl-wynen Oct 28, 2022

SimonHeybrock Oct 28, 2022

jl-wynen Oct 28, 2022

jl-wynen Oct 28, 2022

jl-wynen Nov 3, 2022

SimonHeybrock Nov 3, 2022

		pass


		NXcanSAS.classes = [SASroot, SASentry, SASdata, SAStransmission_spectrum]

		@@ -2,7 +2,7 @@
		# Copyright (c) 2022 Scipp contributors (https://github.com/scipp)

	def dims(group: NXobject) -> Tuple[str]:
	def dims(group: NXobject) -> Tuple[str, ...]:

		self.data = data
		assert Q_variances in (None, 'uncertainties', 'resolutions')

Application definitions #63

Application definitions #63

Conversation

SimonHeybrock commented Oct 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jl-wynen commented Oct 18, 2022

SimonHeybrock commented Oct 24, 2022 • edited

SimonHeybrock commented Oct 25, 2022

SimonHeybrock commented Oct 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonHeybrock commented Oct 17, 2022 •

edited

SimonHeybrock commented Oct 24, 2022 •

edited