WIP: sync_offset, reshape datasets and get coords for UDFs

Google Summer of Code '20 project
Fixes #441, fixes #256, fixes #553 and fixes #726

The Python API and GUI now allow specifying nav_shape and sig_shape parameters to set a different shape than the layout in the dataset
scan_size and detector_size parameters are deprecated in favor of nav_shape and sig_shape respectively
All datasets handle missing data gracefully as black frames
The Python API and GUI now allow specifying a sync_offset to handle synchronization/acquisition problems
Users can now access the coordinates of a tile/partition slice through UDFMeta.coordinates

Todo:

[ ] Fix bug for sig_shape reshaping in HDF5 and write tests
[ ] Implement sync_offset in HDF5 and write tests to check working with ROI and reshaping
[ ] Handle missing data properly for HDF5 and RAW
[ ] Implement sync_offset for K2IS
[ ] Allow loading K2IS time series
[ ] Fix bug in DM format
[ ] Client side improvements to handle scan_size and detector_size deprecation
[ ] Get additional info for each dataset in a HDF5 file
[ ] Look into UDF coords generation and the 'code that allocates result buffers and/or sets the views into them' and try to refactor

Contributor Checklist:

[x] I have added or updated my entry in the creators.json file
[x] I have added a changelog entry for my contribution
[x] I have added/updated documentation for all user-facing changes
[ ] I have added/updated test cases
[ ] I have included the rebuilt production build of the client

Hi @AnandBaburajan !
Thanks for this tool, it helps me a lot! I just tested it with one of my datasets.
Unfortunately, at the moment it is not yet working as I would expect it to.
For me as a user this end_offset field is a bit confusing. I think this is simply offset + scan_size_x*scan_size_y. When I set it to this value it said this would be too large.
The dataset did load with end_offset set to zero.
The analysis seems to ignore the start_offset.

For me as a user this end_offset field is a bit confusing. I think this is simply offset + scan_size_x*scan_size_y.

Oh, I see. Maybe I should rename it to "Number of frames to skip at the end" instead.

The analysis seems to ignore the start_offset and adds zero frames instead.

So this isn't the expected behaviour. I wasn't sure about this functionality that's why I posted the screenshots at Gitter. I've got it now: start_offset number of frames should be 'skipped' from the beginning, not just show black-frames. Thanks!
@sk1p I'll work on this and get back to you if I've any doubts.

@AnandBaburajan Sounds good!
Nevertheless I want to emphasize again that at least in the GUI you won’t specify how many frames will be skipped at the end but that this will happen automatically if offset + scan_size_x * scan_size_y results in less frames than the total.

Nevertheless I want to emphasize again that at least in the GUI you won’t specify how many frames will be skipped at the end but that this will happen automatically if offset + scan_size_x * scan_size_y results in less frames than the total.

Agreed! It would be good to show that there will be frames ignored at the end (and how many), though, so people can check if the given parameters do what the users expects

@sk1p I've made the suggested changes and the offset works as expected now.

I've made the suggested changes and the offset works as expected now.

Nice, I'll have a look!

it should be possible to implement the offset by changing the start_frame and num_frames arguments of the partition, in relation to the part_slice.

I think a previous version of this modified get_partitions and make_slices - I would prefer that approach!

That was my initial approach but it only replaced offset number of frames with black/dark/zero frames. offset number of frames were not skipped from the beginning as expected, like jhgee said:

The analysis seems to ignore the start_offset and adds zero frames instead.

Then I realized it made more sense to specify the offset in the dataset itself, before the partitions are formed.

Fixing this means to implement the feature at the right abstraction level, which IMHO is where the Partitions are created: there, the DataSet coordinates are mapped down to frame indices

I see, maybe I did something wrong with the offset and partitioning, I'll surely try again. Thanks for the feedback!

I think a previous version of this modified get_partitions and make_slices - I would prefer that approach!

That was my initial approach but it only replaced offset number of frames with black/dark/zero frames. offset number of frames were not skipped from the beginning as expected

I had a look at the old version; I think this was caused by also adjusting the part_slice along with the start/stop values. It works like this: the part_slice defines the flattened navigation coordinates that make up the Partition, and start/stop define the indices of the underlying data; the indices of the frames/images in the FileSet. For example:
In [8]: ds
Out[8]: <SEQDataSet of uint16 shape=(8, 8, 128, 128)>

In [9]: list(ds.get_partitions())
Out[9]:
[<BasePartition [0:10]>,
 <BasePartition [10:20]>,
 <BasePartition [20:30]>,
 <BasePartition [30:40]>,
 <BasePartition [40:50]>,
 <BasePartition [50:60]>,
 <BasePartition [60:64]>]

The numbers in brackets denote the start_frame and start_frame + num_frames of the Partition. And:
In [11]: p0 = next(ds.get_partitions())

In [12]: p0._start_frame
Out[12]: 0

In [13]: p0._num_frames
Out[13]: 10

In [14]: p0.slice
Out[14]: <Slice origin=(0, 0, 0) shape=(10, 128, 128)>

You see that, without defining an offset, the slices perfectly match the start/stop values. Now let's ask p0 for some data:
In [16]: from libertem.io.dataset.base import TilingScheme
In [17]: from libertem.common import Shape, Slice
In [19]: tiling_scheme = TilingScheme.make_for_shape(tileshape=Shape((3, 128, 128), sig_dims=2), dataset_shape=ds.shape)
In [22]: tile = next(p0.get_tiles(tiling_scheme))
In [23]: tile
Out[23]: <DataTile <Slice origin=(0, 0, 0) shape=(3, 128, 128)> scheme_idx=0>

So the first tile starts at origin 0. But what happens if we change both slice and the starting frame index associated with p0? Let's monkey-patch around a bit:
In [30]: p0.slice = Slice(origin=(5, 0, 0), shape=p0.slice.shape)
In [31]: p0._start_frame = 5
In [32]: p0
Out[32]: <BasePartition [5:15]>

Now, p0 maps to the frames 5 to 15 in the underlying FileSet. But also:
In [33]: tile = next(p0.get_tiles(tiling_scheme))

In [34]: tile
Out[34]: <DataTile <Slice origin=(5, 0, 0) shape=(3, 128, 128)> scheme_idx=0>

The first tile we request from p0 now has its origin in DataSet coordinates at index 5! Let's imagine we modify all partitions accordingly, and add a 5 frame offset. No other partition offers data for indices 0 to 4 anymore, so:

The analysis [...] adds zero frames

(we can't really see the relationship between p0._start_frame and the indices into the FileSet here, for that one would have to dive a bit deeper into the source)

Hi @AnandBaburajan I just tried out your new implementation. Ignoring frames at the end works fine now, but I get errors when setting an offset.

Hi @AnandBaburajan I just tried out your new implementation. Ignoring frames at the end works fine now, but I get errors when setting an offset.

@jhgee Can you tell me the offset specified? Thanks! I'll look into it and I'm also working on a better approach.

I specified 1742 frames.

@sk1p Thank you soo much for the help! The negative offset works too! Please let me know your suggestions. :)

Hi @AnandBaburajan I just tried out your new implementation. Ignoring frames at the end works fine now, but I get errors when setting an offset.

@jhgee Thanks to you too! Please check again, whenever you're free.

Looks very good! Thanks @AnandBaburajan !
I couldn't find an error yet. I wanted to test this also from the Python API. Does this work already?
The only thing I noticed in the GUI is the following:
You can't type type negative offsets, like -2 directly.

I wanted to test this also from the Python API. Does this work already?

I think so, but I'll wait for sk1p for his suggestions.

You can't type type negative offsets, like -2 directly.

I'll fix that, thanks.

We could take the opportunity and fill missing frames at the end too.

I wanted to test this also from the Python API. Does this work already?

I think so, but I'll wait for sk1p for his suggestions.

You can already test this, using the sync_offset parameter when opening the data. Just keep in mind that this may change before the PR is merged.

@sk1p Sounds good!
With the offset as another parameter for the data set I thought about implementing the sidecar file idea from #768

With the offset as another parameter for the data set I thought about implementing the sidecar file idea from #768

Yeah, that would be helpful. Especially in a multi-user scenario, as the LiberTEM GUI only caches parameters in the user's browser.

We could take the opportunity and fill missing frames at the end too.

@sk1p Should I go ahead with filling missing frames at the end and removing the error?

Should I go ahead with filling missing frames at the end and removing the error?

Sure - as long as that number then is also displayed in the form(s)

I wanted to test this also from the Python API. Does this work already?

I've tested this too and sync_offset works as expected.

Should I go ahead with filling missing frames at the end and removing the error?

I'm not finished with this yet.

Regarding the low level details of sync_offset, they appear fine, but I would love to have some test cases for them, which would make it easier to check, and also to keep it valid in the future.

👍

What is the reason for disallowing cancelling the dialog via escape key or clicking on the "curtain" around the modal? This is usually expected behavior for modals. If you think users would close the modal accidentally, maybe instead the fine-tune form could be directly integrated into the "main" form? It should not take up much more place in the form, right? It can still be its own component, of course, and possibly visibly separated from the other parameters, for example by a headline. Let me know what you think.

If the user changes the offset but escapes the modal without clicking cancel/update, I think he/she might not be sure whether the offset change was updated or cancelled. Regarding going with the settings modal or the form, I'm fine either way. I think a bit more lengthy form would be better than having to click two extra buttons. But I don't know how often users will use offset correction and the upcoming dimension feature. Please let me know your opinion. :)

A general question, can't the fine-tune dialog use formik validation? That way, the validation errors would be consistent between the main form and the fine-tune dialog.

Yup, that'd be better.

Just letting you know that I'm working on MemoryDataSet now and will add tests next.

Perfect! If you like, you can also add tests for the SEQ format - you can set them up such that they expect to find the small 8x8 file in data/SEQ/2020-05-04 8x8 8fps 128x128 streampix 8.seq or similar, and only run if the data is present (see the MIB tests for an example). The tests relying on test data are not organized very well yet, so it's fine if you only add tests for the functionality you added.
I also promised to give you my thoughts on the API. I'm thinking that the synchronization, together with the reshaping in the future, should be handled the same way for all data sets. It could look a bit like this:
ctx.load("mib", path="...", shape={  # or maybe call the parameter `reshape`?
    "nav": (6, 8),
    "sig": (256, 256),  # or whatever
    "sync_offset": -2,
})
# under the hood, this instantiates and initializes a MIBDataSet:
ds = MIBDataSet(path=..., shape=...)
ds = ds.initialize(...)

With all keys of shape being optional. At the beginning, only sync_offset would be supported, later other parameters for the reshaping feature can be added there. The DataSet subclasses, like MIBDataSet above, can then just pass shape down to the superclass in their __init__, where the validation etc. can be handled.
What do you think?
Note, maybe the make_slices call can also be put into common code? I think it will look almost the same for all DataSet implementations - maybe something to look at after implementing support in the other DataSet impls?

If the user changes the offset but escapes the modal without clicking cancel/update, I think he/she might not be sure whether the offset change was updated or cancelled.

True, although it is mitigated a bit by displaying the current parameters in the form.

I think a bit more lengthy form would be better than having to click two extra buttons. But I don't know how often users will use offset correction and the upcoming dimension reshaping feature. Please let me know your opinion. :)

As far as I understand, it depends on the instrument setup - I expect some people will be using this feature a lot (if their setup is a bit experimental/inaccurate/...), while others will only use it in exceptional cases (for example, when loading data from aborted acquisitions). I'm fine with having a more lengthy form.

maybe the make_slices call can also be put into common code? I think it will look almost the same for all DataSet implementations - maybe something to look at after implementing support in the other DataSet impls?

Noted.

A general question, can't the fine-tune dialog use formik validation? That way, the validation errors would be consistent between the main form and the fine-tune dialog.

I tried but faced some issues:

For the client-side validation of sync_offset, the json schema in seq.py would need to dynamically change depending on scan_size.
For custom validation, the schema must be dynamic and described in the client, using Yup or similar. Custom validation could be done for sync_offset without a schema (if I understood correctly), but we use Ajv with schemas right now.
I was not able to pass errors from the main form and handle them in the Settings component. Also, I didn't find much resources to learn about nested Formik components and I found that it's creator doesn't recommend nested form elements or handling errors manually.

Right now, the client handles basic validation (number and not empty) and the offset check happens after form submission. But now the older validation check with useState can't be used because that would require passing handleSubmit handlers between the nested forms.
@sk1p Please have a look at the GUI and let me know your suggestions, whenever you're free. Thanks! All other changes suggested have been made. I'm working on reshaping now.

Please have a look at the GUI and let me know your suggestions, whenever you're free. Thanks! All other changes suggested have been made.

I'll have a detailed look at the beginning of next week. Maybe these validation changes can also be postponed until after the reshaping part, maybe things become more clear then.

I'm working on reshaping now.

:+1:

One thing I noticed is that if we keep it like this, UDFPreprocessMixin.preprocess doesn't have a chance to initialize result buffers at the positions where the data is missing, which may be a problem

Got it!

so maybe in addition to working on reshaping, it would make sense to start integrating the work with two or three additional dataset implementations

👍

SEQ and MIB are done. I've made changes to solve the UDF preprocess issue. Testing K2IS's sync flag right now. Will look into other formats and common code next.

I've written an approach for reshaping using _get_reshaped_tile_slice in HDF5DataSet. A 3D HDF5 dataset with flattened nav dimensions can be loaded now, but I'm yet to write prototypes for n-D datasets.
In [1]: ds = ctx.load("HDF5", path="X:/0/hdf5/3d_dataset.h5", ds_path="ds_path", reshape={"scan_size": (100, 84), "detector_size": (250, 250)})

I think I'll work on the ROI part once the approach is approved. Also, I haven't implemented reshaping into the GUI yet, just the Python API.
@sk1p please check whenever you're free, thanks! :)

Sorry, it took a while, as we had to do an urgent release to fix some compatibility issues... a few quick comments, I'll try to go into more detail later today.

The goal is to allow a user to load any n-D dataset as 4D, so can we just have the option suggesting "Dataset detected is n-D, do you want to reshape it to 4D?", instead of asking for the scan and detector dims?

Hmm, why the extra step of asking the user? They'll need to reshape either way, right?

Also, some formats in the GUI will have two scan_size fields, one for specifying number of frames and one for reshaping, which might be confusing.

An idea would be to remove the old scan_size parameters completely (well, while taking care of backwards compat. etc.). So if no reshaping is done, we could just default to a 1D navigation shape (mostly useful for use by the Python API), and if the user needs to reshape the data, they can use your new interface.

Regarding the GUI forms, the validation is working. The issue is with displaying the ErrorMessage and other users are facing this issue too.
https://stackoverflow.com/questions/62095966/using-nested-object-names-in-formik-errormessage

Hmm, I see. Note that we are using an older version of formik in libertem, as I didn't find the time (or a reason) to upgrade yet. But I guess the problem is still present in the current version, as that stackoverflow question is quite recent?
One thing I looked into before is using https://react-hook-form.com/ instead of formik, if you like, you can have a look at porting our forms over. What do you think?

An idea would be to remove the old scan_size parameters completely (well, while taking care of backwards compat. etc.). So if no reshaping is done, we could just default to a 1D navigation shape (mostly useful for use by the Python API), and if the user needs to reshape the data, they can use your new interface.

Now I'm thinking about not having a 'reshape' feature at all. What if I:

Allow setting scan_size for all the formats (as missing frames can be handled now)
Integrate reshaping into the current scan_size
Mention that LiberTEM can reshape the data through the scan_size parameter if needed in the GUI and docs.

IIUC, setting detector_size isn't required at all as users don't change that (except for raw). Regarding sync_offset, it can exist on its own without reshape` as some users use it often. Please let me know your suggestions and if I understood something incorrectly.

One thing I looked into before is using https://react-hook-form.com/ instead of formik, if you like, you can have a look at porting our forms over. What do you think?

👍 - I'll work on the dynamic schema validation too.

Now I'm thinking about not having a 'reshape' feature at all. What if I:

Allow setting scan_size for all the formats (as missing frames can be handled now)
Integrate reshaping into the current scan_size
Mention that LiberTEM can reshape the data through the scan_size parameter if needed in the GUI and docs.

To clarify my point, an example using the API: for a 3D dataset (256, 16, 16), if the user sets scan_size as (256), it will be loaded as it is. If (16, 16) is set as the scan_size, it will automatically load as 4D. If (20, 16) is set, it will handle the missing frames after loading as 4D. Regarding the GUI, as the user would know that scan_size can reshape their 3D dataset into 4D, they would specify either (16, 16), or (20, 16) or sth if there are missing frames.
IIUC, setting detector_size isn't required at all as users don't change that (except for raw). Regarding sync_offset, it can exist on its own without reshape as some users use it often. Please let me know your suggestions and if I understood something incorrectly.

I'll need to think a bit about this, I'll come back to this later.

any other reason to port except the ErrorMessage issue?

I quite like what I've seen of the API, other than that, not much - should be roughly equivalent in terms of features. As hook form is based on hooks, it's possibly easier in general to re-use parts of forms etc. The performance figures cited in the documentation are also quite nice, but I doubt they matter much for our small forms.

One thing I noticed is that if we keep it like this, UDFPreprocessMixin.preprocess doesn't have a chance to initialize result buffers at the positions where the data is missing, which may be a problem (although I don't think we have a user of preprocess yet, at least not as part of LiberTEM).

I believed I had fixed this issue last week but I only handled missing data at the end case. I didn't think about the case when blank frames were inserted by setting a -ve offset. So no changes must be made to the partition slices at all.
I've written a prototype for setting a +ve offset for HDF5 in get_tiles but right now it only works for simple 4D datasets like (16, 16, 16, 16) for example, and there's a small issue: the 'blank' frames aren't blank and I'll work on it. It can definitely be improved but I'd like your feedback on the approach (whenever you're free) before I go ahead with generalizing it for n-D datasets. If the approach in HDF5 is okay, I think a similar solution can be written for all the other formats' default get_tiles and K2IS's sync flag issue can be handled too if get_tiles is modified instead of the partitions.
So I'll generalize sync_offset and reshape, if both their approaches in HDF5 are okay on your side. Thanks for your feedback! I think I should work on the roi part and fix the additional info issue till then.

Here is the first part of my review, sorry I didn't get very far, but I'll continue later today.

@sk1p I just wanted to let you know that only the reshaping and +ve sync offset prototype in HDF5 work right now (mentioned in my previous comments in detail). I will work again on seq, k2is, etc after you approve my functions in hdf5. Is that okay? Then I can create similar functions for the default get_tiles for rest of the formats.

Sorry it took so long, I'll try to give more timely feedback in the future.

I just wanted to let you know that only the reshaping and +ve sync offset prototype in HDF5 work right now (mentioned in my previous comments in detail). I will work again on seq, k2is, etc after you approve my approaches in the functions in hdf5. Is that okay? Then I can create similar functions for the default get_tiles for rest of the formats.

One thing to keep in mind is that the HDF5 DataSet is kind of an outlier - internally it works quite differently from other formats, as we don't do the low-level I/O, but instead (have to!) let h5py handle it. I think it would be best to look at one additional format, like SEQ or MIB, so we can see how it will interact with the low-level I/O code (read ranges etc.)

I'm also looking forward to your opinion on integrating reshaping into the current scan_size (mentioned earlier)

The more I think about it, the more I like it. I also kind of liked the idea of "packing" all the reshape/offset-related parameters into their own namespace, but I can see that it would make the implementation a bit harder. We should probably rename scan_size to something more general, though, as it only makes sense for a specific subset of data we can handle (1D line scans or 2D area scans). While they are very common, one reason to implement this reshaping project is to support more general cases. Maybe call them nav_shape and sig_shape instead? For those formats that already have a scan_size parameter, we should handle backwards compatibility and add proper deprecation warnings.

I believed I had fixed this issue last week but I only handled missing data at the end case. I didn't think about the case when blank frames were inserted by setting a -ve offset. So no changes must be made to the partition slices at all.

I think it would be very valuable to have tests for all these cases - showing how the coordinates change on reshape and when applying an offset, both positive and negative. Also adding a test that shows the integration with the UDF interface. You can add all those tests first, and then work one-by-one on how to make them pass. In that way, the tests can be your guide on what to work on next, for example, and you can try multiple approaches and see if they work for all the tested cases. Things can get quite complicated, especially once you factor in features like roi...

I've written a prototype for setting a +ve offset for HDF5 in get_tiles but right now it only works for simple 4D datasets like (16, 16, 16, 16) for example, and
there's a small issue: the 'blank' frames aren't blank and I'll work on it.

My guess: you are yielding an uninitialized buffer (created by np.ndarray)? In case there are empty frames inserted, I don't think it makes much sense to yield data at all - just start yielding tiles where the data starts. Or does that conflict with any of the other requirements?

K2IS's sync flag issue can be handled too if get_tiles is modified instead of the partitions.

Not so sure about that, but I'll let you work on it! Btw. we will probably get support for another file format very soon, which works quite similarly, but should be simpler to understand than K2IS.

I noticed the following discrepancy: when loading SEQ data in the GUI, and reshaping (in this case from 8x8 to 4x4), the number of frames ignored at the end is 0. But there should be 48 extra frames at the end still, one of which could be inserted as the last frame, and the other 47 should be shown as ignored, or am I missing something?

You're right, I haven't finished reshaping for SEQ and I haven't accounted reshaping for the 'number of frames' calculations yet, sorry.

I propose to add nav_shape, sig_shape and sync_offset parameters to all datasets, and properly handle backwards compat. where needed. This should also be less awkward in the client types, I guess they can then be put into DatasetParamsCommon?

Sounds good, I'll work on it! Thanks for the feedback!

You're right, I haven't finished reshaping for SEQ and I haven't accounted reshaping for the 'number of frames' calculations yet, sorry.

No worries, just something to keep in mind!

Sounds good, I'll work on it! Thanks for the feedback!

:+1:

I think it would be very valuable to have tests for all these cases - showing how the coordinates change on reshape

@sk1p I've finished reshaping for HDF5 and for the formats using get_tiles_w_copy. I've written a test for HDF5 (data with flattened nav dimension #441 ) and it works. It also works with both nav and sig dims flattened. Can you give me one or more examples/use-cases for tests I could write for other formats? Though the feature works for HDF5, I'm not sure if it works for other formats until I get n-D data (K2IS time series, thanks!) and write tests.
Update: I'm working on HDF5's offset and I've to fix a bug I introduced in DM, complete offsets for K2IS, FRMS6 and SER and write tests for all of them. I'll start working on the UDF interface soon.

Sounds right, you could also split into two tiles in this case - the DataSet classes have quite a bit of freedom on how to return the data, in the sense that a single tile can have less frames in it than defined in the tiling scheme, for example for border conditions like this.

@sk1p I thought about splitting tiles based on the offset but I think it'll get complicated for higher-dimensional data, so I'm also trying to access the data using appropriate indices (similar to how ROI is handled).
Can you please take a look at the coordinate generation in the UDF code and tell me if something's wrong (I've added tests with ROI), thanks!
Now things left to do:

Fix sync_offset for HDF5 and make sure it works with reshaping and roi
Make sure FRMS6's dark frames correction works properly with sync_offset
K2IS's sync flag
Setting a sync_offset in RAW and HDF5 data results in green but empty frames in the GUI, but not in notebooks
Documentation (and more helpful comments)

Thanks, the tests look good! I've added some comments about the implementation, maybe @uellue can also add his comments about the interface itself.

In what format are the coordinates available? List of indices in the nav shape? As already mentioned, it might be smart to do this on-demand since most UDFs don't need the information.

Another point: It feels like this is somewhat similar to the code that allocates result buffers and/or sets the views into them? Maybe there's an opportunity to create synergies between the two?

Is this already ready for testing? I could perhaps see if it is useful for the SSB Ptychography UDFs to get a feeling about the API. :-)

Another point: It feels like this is somewhat similar to the code that allocates result buffers and/or sets the views into them? Maybe there's an opportunity to create synergies between the two?

That's a good point - @AnandBaburajan do you want to look into this, or should I take a look at refactoring after the project is done?

Another point: It feels like this is somewhat similar to the code that allocates result buffers and/or sets the views into them? Maybe there's an opportunity to create synergies between the two?

That's a good point - @AnandBaburajan do you want to look into this, or should I take a look at refactoring after the project is done?

Yup, I'll take a look

@sk1p I thought about splitting tiles based on the offset but I think it'll get complicated for higher-dimensional data, so I'm also trying to access the data using appropriate indices (similar to how ROI is handled).
Okay, makes sense if this reduces the complexity - I guess the performance doesn't suffer too much if you are using read_direct and read into the re-used tile buffer?

@sk1p For sync_offset, using just the indices to read was a bad idea as it's x10 slower (for huge datasets) than using read_direct. So I think the best option would be, like you said, to split tiles, even if it gets complicated (maybe) for higher-dimensional data. I'm still working on it :-)

Fix sync_offset for HDF5 and make sure it works with reshaping and roi
I guess also the general nD-case in HDF5 was still WIP?

Yup.

Make sure FRMS6's dark frames correction works properly with sync_offset
How does the dark frame correction come into contact with sync_offset? Or do you mean just to make sure no sync_offset is applied when reading the dark frame data from the first file?

I thought sync_offset has to be applied while reading the dark frames too! I've got it now, thanks! By the way, test_corrset.py' in benchmarks/corrections' threw some errors (master branch) but I haven't looked into it.

K2IS's sync flag
Please do let me know if you have any questions, and if you have a look at the test data I sent you!

I couldn't load both the datasets because the metadata didn't have .SI Dimensions.Size Y and .SI Dimensions.Size X. So I looked through the metadata but couldn't find anything related to scan size or number of frames. I found .Acquisition.Padded Frame Count': 10 but I'm not sure if it's actually the frame count.

For sync_offset, using just the indices to read was a bad idea as it's x10 slower (for huge datasets) than using read_direct. So I think the best option would be, like you said, to split tiles, even if it gets complicated (maybe) for higher-dimensional data. I'm still working on it :-)

Okay, sounds good, thanks for looking into the performance. Did you also try to combine the two, that is, issue a read_direct call for each navigation index? Something like this:
dataset.read_direct(buf[index_in_tile], source_sel=slice_in_ds_coords)

This could then also potentially be used to build a better performing version of _get_tiles_with_roi.

I thought sync_offset has to be applied while reading the dark frames too! I've got it now, thanks!

By the way, test_corrset.py in benchmarks/corrections threw some errors (master branch) but I haven't looked into it.

You got those errors on the master branch? Feel free to create an issue for that, so it doesn't get lost. We did some work on this at the end of June, so it's possible we broke something there...

K2IS's sync flag
Please do let me know if you have any questions, and if you have a look at the test data I sent you!

I couldn't load both the datasets because the metadata didn't have .SI Dimensions.Size Y and .SI Dimensions.Size X. So I looked through the metadata but couldn't find anything related to scan size or number of frames. I found .Acquisition.Padded Frame Count': 10 but I'm not sure if it's actually the frame count.

Yeah, the missing Dimensions was what I expected. You could try to look at all DataBlocks and check their frame_id values - the number of unique frame_id should be the number of frames in the data set. So It's possible that to find out the "real" number of frames in the raw data, one has to read the first and last block and check the frame_id, possibly accounting for different first block offsets (see K2Syncer.sync_sectors). You can then also compare that to the "Padded Frame Count" metadata item. Let me know if that works out!

get_coordinates in the UDF code is not as usable as before, as users need to think/look up what to pass to the get_coordinates function, even though it is all available on the UDFMeta instance.

Ah, got it.

What do you think about instead making UDFMeta.coordinates a cached property? So on the first call, if coordinates are not yet available, get_coordinates is called under the covers, and on subsequent calls, a cached value will be used. Just need to be careful, as the cache needs to be invalidated when slice, dataset_shape or roi changes. This way, the user doesn't have to pay any performance penalty if they don't access the property, and the interface is very simple again. What do you think?

That would be much better than the current one, thanks!!

One more thing I noticed, not related to the latest changes, is some behavior that can be improved. Namely, when opening a data set in the GUI where we have some parameters cached in local storage from an older LiberTEM version, where the shape parameters weren't there yet, these fields (Nav shape, sig shape) are empty. So either the user has to be clever enough to press the reset button, but possibly losing some other parameters, or they have to know the nav/sig shape, even though in some cases it can be detected.
Maybe this could be improved by preferring detected nav/sig shape values over empty fields from stored parameters?
On a related note, in the raw data set, the stored parameters may have the old scan_size/detector_size names, they should be upgraded to the new names. Maybe we need some per-dataset-type mechanism for merging detected and stored parameters? Maybe you can have a look into this.

👍
One more thing left to do: while opening HDF5 in the GUI, the nav_shape, sig_shape, sync_offset and image_count have to be detected and changed automatically if the user selects a different dataset from the dropdown.

One more thing left to do: while opening HDF5 in the GUI, the nav_shape, sig_shape, sync_offset and image_count have to be detected and changed automatically if the user selects a different dataset from the dropdown.

Ah, that's a good point :+1: I guess the shape can be included in the info part of the detected parameters?

Using read_directs for each index was slow too, so I went with splitting tiles.

Okay, thanks for looking into it!

Currently only the nav_shape can be reshaped and extra/missing frames is handled too. There are some bugs to be fixed for sig_shape reshaping. Also HDF5's sync_offset implementation is not yet ready but I think it can be done quickly since tiles can be split now. Please let me know your opinion on the slice splitting method, thanks!

I'll have a look and add some comments.

What do you think about instead making UDFMeta.coordinates a cached property?

I used a simple dictionary (please check) as functools.cached_property is only available in Python 3.8+

Looks good! :+1:

I also had a look at the tests, in addition to checking the single result frame (for example in test_reshape_3D_and_5D_to_4D), the whole result could be checked for equality (like in test_simple_reshape_3D_and_5D_to_4D), which could be helpful as it should include all border conditions.

I just tried doing that but for some reason, the tests are failing. But when I ran the tests on real HDF5 datasets instead of the random-temporary-reshaped datasets, they passed. The results are also fine when ran on real HDF5 datasets in a notebook.

I just tried doing that but for some reason, the tests are failing. But when I ran the tests on real HDF5 datasets instead of the random-temporary-reshaped datasets, they passed. The results are also fine when ran on real HDF5 datasets in a notebook.

Hmm, I'll have a look.

I just tried doing that but for some reason, the tests are failing. But when I ran the tests on real HDF5 datasets instead of the random-temporary-reshaped datasets, they passed. The results are also fine when ran on real HDF5 datasets in a notebook.

Just had the chance to look at this, the tests in the branch seem to pass. Can you maybe push the failing test cases?

Can you maybe push the failing test cases?

I can't push right now, but here's the test case:
def test_reshape_3D_to_4D(
    lt_ctx, hdf5_same_large_data_4d, hdf5_same_large_data_3d
):
    ds = H5DataSet(
        path=hdf5_same_large_data_4d.filename,
        ds_path="data",
        nav_shape=(79, 55),
    )
    ds = ds.initialize(lt_ctx.executor)
    analysis = lt_ctx.create_disk_analysis(
        dataset=ds, cx=125, cy=125, r=125
    )
    result = lt_ctx.run(analysis)
    result = result['intensity'].raw_data

    ds_actually_3d = H5DataSet(
        path=hdf5_same_large_data_3d.filename,
        ds_path="data",
        nav_shape=(79, 55),
    )
    ds_actually_3d = ds_actually_3d.initialize(lt_ctx.executor)

    analysis_ds_actually_3d = lt_ctx.create_disk_analysis(
        dataset=ds_actually_3d, cx=125, cy=125, r=125
    )
    result_ds_actually_3d = lt_ctx.run(analysis_ds_actually_3d)
    result_ds_actually_3d = result_ds_actually_3d['intensity'].raw_data

    assert np.allclose(result, result_ds_actually_3d)

Thanks.

Also HDF5's sync_offset implementation is not yet ready but I think it can be done quickly since tiles can be split now

Can this be integrated into _get_reshaped_tiles, such that combinations of offset and reshaping are also easily possible?

+ve sync_offset for HDF5 works good now but the combination of offset and reshaping isn't finished yet.

I just tried doing that but for some reason, the tests are failing. But when I ran the tests on real HDF5 datasets instead of the random-temporary-reshaped datasets, they passed. The results are also fine when ran on real HDF5 datasets in a notebook.

I'm facing this issue while applying sync_offset too and I'm looking into it. It's strange how the reshaping and offset tests pass with real data, but fail with random data.

I can't push right now, but here's the test case:
[...]

I've created a reproducer in a notebook, and looking at the absolutes of the differences reveals a pattern:

If you look closely, about every third row seems to match, until we get to the right border, where the last three (?) pixels have a mismatch. I'll send you the notebook via gitter, as I can't easily attach it to the issue comment here...
A good approach to further debug this would be to create structured data instead of random data. For example, you could create data in a way that should result in a linear gradient once the analysis is applied, for example by filling the first image in the dataset with constant 0, the next one with constant 1 etc.

Now I've a better understanding on how K2IS's sectors are synced. My implementation works for minor changes to the offset but has some issues for example when sync_offset is set to 0 and all the data without the shutter_active flag is read too, but I'm working on it.

My implementation works for minor changes to the offset but has some issues for example when sync_offset is set to 0 and all the data without the shutter_active flag is read too, but I'm working on it.

Sounds good, let me know if you need to know anything, here or on gitter

I've created a reproducer in a notebook, and looking at the absolutes of the differences reveals a pattern:

If you look closely, about every third row seems to match, until we get to the right border, where the last three (?) pixels have a mismatch. I'll send you the notebook via gitter, as I can't easily attach it to the issue comment here...
A good approach to further debug this would be to create structured data instead of random data. For example, you could create data in a way that should result in a linear gradient once the analysis is applied, for example by filling the first image in the dataset with constant 0, the next one with constant 1 etc.

@sk1p I created structured int32 data the way suggested above. The unit tests passed on that data and all the resultant rows matched in the notebook you provided me. As I mentioned earlier, the reshaping tests passed for real sample data 4DSTEM_experiment/data/datacubes/polyAu_4DSTEM/data too.
With the branch set to the current master, I modified your notebook and removed the nav_shape arguments from H5DataSet initialization so that there's no need for reshaping. So the 4D dataset would be loaded as 4D and the 3D dataset would be loaded as 3D itself. Then I ran the notebook test: it failed surprisingly and the rows did have a mismatch (especially the last three (?) pixels).
So now I think there are some issues with the floating-point arithmetic on the data generated by np.random.randn and I'm not sure where to start looking. I was also curious about the tolerance accepted for floating-point equality checks for LiberTEM, as the errors in my tests are of ~0.0002 on average. Thanks!

So now I think there are some issues with the floating-point arithmetic on the data generated

I'm not so sure about this - the operation should be deterministic, in that the operations should be executed in the same order and yield exactly the same results. You can also try to replace it with an even simpler operation, like summing up the whole frames.
It is weird though that your test with int data works - did you check that you are doing the exact same operation in both tests? Can you try to convert your int test data to float and see if that changes things? Thanks!

Can you try to convert your int test data to float and see if that changes things? Thanks!

I tried, the test (disk analysis) in your notebook passed on int32 data but not on float32.

You can also try to replace it with an even simpler operation, like summing up the whole frames.

I ran the SumSigUDF() on both int32 and float32 data, and both the tests passed!

I tried, the test (disk analysis) in your notebook passed on int32 data but not on float32.
I ran the SumSigUDF() on both int32 and float32 data, and both the tests passed!

Hmm, could you provide these tests? Both using SumSigUDF and the original? I think that would make debugging on my end a bit easier, as I don't have to re-create your setup and data generation. Thanks!

I was having another look at this just now, and tried to have a look at the individual frames / signal elements using picking. I tried the following:
ctx.run(ctx.create_pick_analysis(dataset=ds_actually_3d, x=0, y=0)).intensity

And got this error:

ValueError: cannot reshape array of size 250 into shape (1,250,250)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-19-fbf12b0208d4> in <module>
----> 1 ctx.run(ctx.create_pick_analysis(dataset=ds2, x=0, y=0)).intensity

~/source/upstream-LiberTEM/src/libertem/api.py in run(self, job, roi, progress, corrections)
    634                 if roi is None:
    635                     roi = analysis.get_roi()
--> 636                 udf_results = self.run_udf(
    637                     dataset=analysis.dataset, udf=analysis.get_udf(), roi=roi,
    638                     progress=progress, corrections=corrections,

~/source/upstream-LiberTEM/src/libertem/api.py in run_udf(self, dataset, udf, roi, corrections, progress, backends)
    705         if corrections is None:
    706             corrections = dataset.get_correction_data()
--> 707         results = UDFRunner([udf]).run_for_dataset(
    708             dataset=dataset,
    709             executor=self.executor,

~/source/upstream-LiberTEM/src/libertem/udf/base.py in run_for_dataset(self, dataset, executor, roi, progress, corrections, backends)
   1161         if progress:
   1162             t = tqdm.tqdm(total=len(tasks))
-> 1163         for part_results, task in executor.run_tasks(tasks, cancel_id):
   1164             if progress:
   1165                 t.update(1)

~/source/upstream-LiberTEM/src/libertem/executor/dask.py in run_tasks(self, tasks, cancel_id)
    216
    217         try:
--> 218             for future, result_wrap in dd.as_completed(futures, with_results=True):
    219                 if future.cancelled():
    220                     del self._futures[cancel_id]

~/.virtualenvs/master-libertem/lib/python3.8/site-packages/distributed/client.py in __next__(self)
   4428             with self.thread_condition:
   4429                 self.thread_condition.wait(timeout=0.100)
-> 4430         return self._get_and_raise()
   4431
   4432     async def __anext__(self):

~/.virtualenvs/master-libertem/lib/python3.8/site-packages/distributed/client.py in _get_and_raise(self)
   4419             if self.raise_errors and future.status == "error":
   4420                 typ, exc, tb = result
-> 4421                 raise exc.with_traceback(tb)
   4422         return res
   4423

~/source/upstream-LiberTEM/src/libertem/executor/dask.py in __call__()
     96     def __call__(self, *args, **kwargs):
     97         return {
---> 98             "task_result": self.task(),
     99             "task_id": self.task_id,
    100         }

~/source/upstream-LiberTEM/src/libertem/udf/base.py in __call__()
    883
    884     def __call__(self):
--> 885         return UDFRunner(self._udfs).run_for_partition(
    886             self.partition, self._roi, self._corrections,
    887         )

~/source/upstream-LiberTEM/src/libertem/udf/base.py in run_for_partition()
   1111                 # print("UDF TilingScheme: %r" % tiling_scheme.shape)
   1112                 partition.set_corrections(corrections)
-> 1113                 self._run_udfs(numpy_udfs, cupy_udfs, partition, tiling_scheme, roi, dtype)
   1114                 self._wrapup_udfs(numpy_udfs, cupy_udfs, partition)
   1115             finally:

~/source/upstream-LiberTEM/src/libertem/udf/base.py in _run_udfs()
   1038             xp = cupy_udfs[0].xp
   1039
-> 1040         for tile in tiles:
   1041             self._run_tile(numpy_udfs, partition, tile, tile, roi)
   1042             if cupy_udfs:

~/source/upstream-LiberTEM/src/libertem/io/dataset/hdf5.py in get_tiles()
    483
    484         if roi is not None:
--> 485             yield from self._get_tiles_with_roi(roi, dest_dtype)
    486         elif self.meta.sync_offset != 0 or self.native_shape != self.meta.shape:
    487             yield from self._get_reshaped_tiles(tiling_scheme, tileshape_nd, dest_dtype)

~/source/upstream-LiberTEM/src/libertem/io/dataset/hdf5.py in _get_tiles_with_roi()
    454                     shape=result_shape,
    455                 )
--> 456                 tile_data = h5ds[idx].reshape(result_shape)
    457                 self._preprocess(tile_data, tile_slice)
    458                 yield DataTile(

ValueError: cannot reshape array of size 250 into shape (1,250,250)

I realize now that picking frames takes a different branch in the HDF5 dataset code, so this is an unrelated problem.
Other than that, I changed the parameters of your test code a bit, using this as an input:
data = np.linspace(0, 1, 100*84*64*64, dtype=np.float32).reshape((100, 84, 64, 64))

And then using cx=32, cy=32, r=25 as parameters for the disk analysis. Then, plotting the differences using the following...
plt.figure()
plt.imshow(r2 != r1)

Reveals the following pattern:

The first defect is at (1, 28), which is the 82nd element if we flatten the coordinates. So it is close to the border of the original data, before reshaping. Does that give you a hint what the problem could be?
Another approach to get a handle on this problem would be, instead of comparing two results using the dataset and other libertem facilities, to compare to an externally computed "ground truth", like manually applying the mask. You can get the disk mask from the disk analysis via something like analysis.get_mask_factories()[0](); then you could do something like the _naive_mask_apply helper in our test suite.

So now I think there are some issues with the floating-point arithmetic on the data generated by np.random.randn and I'm not sure where to start looking.

Another idea is to save the

I was also curious about the tolerance accepted for floating-point equality checks for LiberTEM, as the errors in my tests are of ~0.0002 on average. Thanks!

So now I think there are some issues with the floating-point arithmetic on the data generated by np.random.randn and I'm not sure where to start looking.

Another idea is to repeat the same test with the raw dataset - there, setting the shape was supported before. So something like this:
rawfn = "/tmp/same-data-as-float32.raw"
with open(rawfn, "w") as rawf:
    data.tofile(rawf)
ds_raw = ctx.load("raw", path=rawfn, dtype=np.float32, sig_shape=(64, 64), nav_shape=(79, 55))
ds_raw_2 = ctx.load("raw", path=rawfn, dtype=np.float32, sig_shape=(64, 64), nav_shape=(79, 11, 5))
da = ctx.create_disk_analysis(dataset=ds2, cx=32, cy=32, r=25)
da_raw = ctx.run_udf(dataset=ds_raw, udf=da.get_udf())['intensity'].data.squeeze()
da_raw_2 = ctx.run_udf(dataset=ds_raw_2, udf=da.get_udf())['intensity'].data.reshape((79, 55))

I just tried this and the two results passed an allclose test. That means we should also be able to pass it with the HDF5 data.
The result was different from both results from the HDF5 testcase in the notebook:

Maybe it makes sense to now go an abstraction layer lower, and check the output of get_tiles directly? One possibility would be to use a smaller data set for this, and/or to fill each frame with its index:
# adjust shape accordingly
data_indexes = np.zeros((100, 84, 64, 64))
data_indexes[:] = np.arange(0, 100*84).reshape((100, 84, 1, 1))  # broadcast navigation indices over the whole frames
# p.get_tiles() etc.. - need to construct a tiling scheme that is not as deep as the default, if a smaller ds is constructed

I didn't manage to reproduce the problem with this construct yet, but maybe it can help anyways

I was also curious about the tolerance accepted for floating-point equality checks for LiberTEM, as the errors in my tests are of ~0.0002 on average. Thanks!

Seems like I missed this question before: usually we try to satisfy the default conditions of np.allclose. But this really depends on the operation - for some, you cannot satisfy this with completely random input data, as sometimes things average out and produce very small results, where you can observe loss of significance. For that reason, we have the _mk_random function for generating test data that is a bit more realistic.
In this case, though, the operation should be deterministic, and even if there is loss of significance with the random input data, it should happen in the same way, regardless of nav_shape - as demonstrated above with the raw dataset.

@sk1p I figured out a much simpler way to deal with HDF5 - without any splitting or adjusting tileshape, but I'm getting this error in udf/base.py in merge(self, dest, src).
ValueError: could not broadcast input array from shape (8,1) into shape (0,1)

    672         for k in dest:
    673             check_cast(dest[k], src[k])
--> 674             dest[k][:] = src[k]
    675
    676     def get_preferred_input_dtype(self):

Have you come across this error before? I understand this the part where the results are merged but since my tile_slices and the yielding code looks fine, I'd appreciate some help. Thanks!
Regarding the earlier float/hdf5/raw/disk-analysis issue, I wrote a test to compare results of a disk analysis on RAW and HDF5 on the same float32 data, without any reshaping, on the master branch, and the test failed (defects at the border).
I ran the same test but with a SumSigUDF instead of a disk analysis. and it test passed. I'm still looking into it

Regarding the earlier float/hdf5/raw/disk-analysis issue, I wrote a test to compare results of a disk analysis on RAW and HDF5 on the same float32 data, without any reshaping, on the master branch, and the test failed (defects at the border).
I ran the same test but with a SumSigUDF instead of a disk analysis. and it test passed. I'm still looking into it

Interesting, thanks for the update! So something else is going on here, maybe related to the HDF5 library or the HDF5 DataSet implementation. As this behavior also exists in master, maybe we can postpone a fix for this after the PR is merged? Keep the testcase as xfail. It's also worthwhile to look into alternative ways of reading from HDF5 files, if only for debugging this issue (sadly we still need the h5py way of access to deal with chunked/compressed/... HDF5 data).

I figured out a much simpler way to deal with HDF5 - without any splitting or adjusting tileshape

This sounds great! Can you push your new code, even if it doesn't work yet? If you don't want to "overwrite" the other approach, you can of course also push to a different branch.

Have you come across this error before? I understand this the part where the results are merged but since my tile_slices and the yielding code looks fine, I'd appreciate some help.

Could you try to run your test case in a debugger (for example via pytest --pdb) and check the values of dest[k].shape, k, src[k].shape etc.? For me it looks like there is an issue with the view that is set on dest, which in turn depends on the current tile/partition/... slice - check the UDFData.set_view_for_* methods, and in turn the BufferWrapper.get_view_for_* methods. In this area (libertem.udf), @uellue should also be able to support you.

@AnandBaburajan since @sk1p is on vacation and I don't have a full overview of this PR: Can I help you with something, do you need input of some kind?

@AnandBaburajan since @sk1p is on vacation and I don't have a full overview of this PR: Can I help you with something, do you need input of some kind?

No, thanks! I'm looking into the HDF5 issues now.

@uellue Not a doubt, but just something I noticed:
Is https://github.com/LiberTEM/LiberTEM/blob/master/src/libertem/udf/base.py#L1004 correct or is it supposed to be udf.set_views_for_partition(partition)? (I'm not much familiar with the UDF parts yet)

@uellue Not a doubt, but just something I noticed:
Is https://github.com/LiberTEM/LiberTEM/blob/master/src/libertem/udf/base.py#L1004 correct or is it supposed to be udf.set_views_for_partition(partition)? (I'm not much familiar with the UDF parts yet)

Yes, this is correct. Good eye, anyway! set_views_for_partition() is used in the merge phase in  UDFRunner.run_for_dataset(). process_partition() is pretty much like process_tile(), only that it processes the data in one big single tile instead of many small ones. For that reason set_views_for_tile() works here although the tile is, in fact, the whole partition. set_contiguous_views_for_tile() which is now used for process_tile() was bolted on later to fix https://github.com/LiberTEM/LiberTEM/issues/739.

I figured out a much simpler way to deal with HDF5 - without any splitting or adjusting tileshape

This sounds great! Can you push your new code, even if it doesn't work yet? If you don't want to "overwrite" the other approach, you can of course also push to a different branch.

It worked! I made some changes to the partitioning this time. Just like earlier, the tests work on int and the sample data, SumSigUDF but fail with float data and on a disk analysis. I'll work on sig_shape reshaping and sync_offset next.
Also, @sk1p should I keep my common.Slice.split function here or should I remove it since it isn't used anymore? Thanks.

It worked! I made some changes to the partitioning this time. Just like earlier, the tests work on int and the sample data, SumSigUDF but fail with float data and on a disk analysis. I'll work on sig_shape reshaping and sync_offset next.

This is great, thanks for the update! I'll have a look at the implementation ASAP.

should I keep my common.Slice.split function here or should I remove it since it isn't used anymore?

I'm fine with removing unused code - if we keep the commit history of this PR, it can still be recovered in the future if needed.

It worked! I made some changes to the partitioning this time. Just like earlier, the tests work on int and the sample data, SumSigUDF but fail with float data and on a disk analysis. I'll work on sig_shape reshaping and sync_offset next.

Sorry for the delay - I only now found time to look at this again.
Could you add a comment in get_partitions that describes the approach and why it works? That would be very helpful! Same goes for the _get_tiles_normal function.
Regarding tests: in CI only the test_positive_sync_offset test fails - locally, I also get a failure for test_reshape_with_different_shapes. Is this expected, i.e. #880? If so, if would be great to also mark that as xfail. Another way of testing would be, in addition to applying an analysis, test that the raw data is allclose. I just did something like this in a jupyter notebook:
for shape in [
    (7, 32, 32),
    (1, 32, 32),
    (3, 32, 32),
    (5, 32, 32),
    (11, 32, 32),
    (13, 32, 32),
    (17, 32, 32),
    (19, 32, 32),
    (23, 32, 32),
    (25, 32, 32),
    (7, 16, 32),
    (64, 3, 32),
]:
    shape = Shape(shape, sig_dims=2)
    tiling_scheme = TilingScheme.make_for_shape(shape, ds.shape)
    print(tiling_scheme)
    all_the_data = np.zeros(ds.shape)
    all_data_flat = all_the_data.reshape((-1, 32, 32))
    for part in ds.get_partitions():
        for t in part.get_tiles(tiling_scheme=tiling_scheme):
            # make sure the data that is read is equal to the source data:
            assert np.allclose(t, test_data_flat[t.tile_slice.get()])
            all_data_flat[t.tile_slice.get()] = t
    # make sure all data is read:
    assert np.allclose(all_data_flat, test_data_flat)

test_data_flat is the generated input data, reshaped to 3d. The outer for-loop could be converted into a parametrize marker.
 Is it possible to have the same tests with smaller data, and only have the large data generated for benchmarks? That would be great!

Regarding tests: in CI only the test_positive_sync_offset test fails - locally, I also get a failure for test_reshape_with_different_shapes. Is this expected, i.e. #880?

No, test_reshape_with_different_shapes passed on my Linux system. So I'll look into this.
Also, I left out to mention something which could be related: although the sync_offset with ROI part isn't finished yet (reshaping is yet to be taken into account), test_negative_sync_offset_with_roi randomly passes and fails on my system.

Also, I left out to mention something which could be related: although the sync_offset with ROI part isn't finished yet (reshaping is yet to be taken into account), test_negative_sync_offset_with_roi randomly passes and fails on my system.

For a test that should fail but passes, maybe a good first step is to make it fail more reliably - right now, the roi is just a single coordinate, maybe this fails more reliably if you use a larger roi? Or put an assert False at the end and run the test with --pdb repeatedly until it reaches that assert, then you can inspect the actual values and data that causes the test case to succeed when it shouldn't.
For using a larger roi, it should be possible to apply the sync_offset to a roi, too (flatten and then np.roll or similar, then reshape back to ds.shape?), and then compare the analysis with sync_offset=0 and the original roi to the result with sync_offset=-123 and the rolled roi.

Discussion with @AnandBaburajan: first get this PR into a mergeable state (excluding full K2IS and HDF5), then work on the other items again. Then also maybe try to use the memory mapping approach

FYI just let me know if I can or should review something!

I've a doubt regarding the sig_shape (detector_size). In some formats, I'm calculating a dataset's meta.image_count using:
image_count = filesize / (dype.itemsize * np.prod(sig_shape))

But in case the user enters a sig_shape != the native_sig_shape, the resultant image_count changes and sometimes becomes float too (when it's not divisible).

Should image_count change when sig_shape changes?
If yes, what should we do when the image_count results in a float?
If no, I should replace sig_shapewith native_sig_shape in the above code right?

Thanks!

I've a doubt regarding the sig_shape (detector_size). In some formats, I'm calculating a dataset's meta.image_count using:
image_count = filesize / (dype.itemsize * np.prod(sig_shape))

Which format is this for? This is only correct for very specific cases, where the data is "dense" (no file/frame/... headers/footers/... etc). - so currently this is mostly the raw format (I remember FRMS6 has similar code, but that should take header counts etc. into account)

But in case the user enters a sig_shape != the native_sig_shape, the resultant image_count changes and sometimes becomes float too (when it's not divisible).

The sig_shape specified by the user should be compatible with the native_sig_shape - that is, it should work like a normal reshape, without cropping off anything. So np.prod(sig_shape) == np.prod(native_sig_shape)

Should image_count change when sig_shape changes?

I don't think so. The main use-case for changing the sig_shape is to read from files where the native shape is not as expected, for example a HDF5 file that has a flat (1D) sig shape for data that is really

If yes, what should we do when the image_count results in a float?

I don't think a float image_count would make sense - the coordinates are always discrete values

If no, I should replace sig_shapewith native_sig_shape in the above code right?

I think if we enforce np.prod(sig_shape) == np.prod(native_sig_shape) this should not matter.

Which format is this for? This is only correct for very specific cases, where the data is "dense" (no file/frame/... headers/footers/... etc). - so currently this is mostly the raw format

Yep, the RAW format and I'm using something similar in SEQ too.

The main use-case for changing the sig_shape is to read from files where the native shape is not as expected, for example a HDF5 file that has a flat (1D) sig shape for data that is really

Thanks, I got confused when I realized that RAW can accept any sig_shape.

I think if we enforce np.prod(sig_shape) == np.prod(native_sig_shape) this should not matter.

Just to clarify, for RAW too right?

Which format is this for? This is only correct for very specific cases, where the data is "dense" (no file/frame/... headers/footers/... etc). - so currently this is mostly the raw format

Yep, the RAW format and I'm using something similar in SEQ too.

Right - for SEQ, the true_image_size header value that is used makes sure the frame headers are accounted for.

The main use-case for changing the sig_shape is to read from files where the native shape is not as expected, for example a HDF5 file that has a flat (1D) sig shape for data that is really

Thanks, I got confused when I realized that RAW can accept any sig_shape.
just remembered that there's no way to get the native_sig_shape for RAW.

Yeah, RAW is a bit of an outlier compared to the other formats.

/azp run libertem.libertem-data

/azp run libertem.libertem-data

Re-running data tests to check coverage:
/azp run libertem.libertem-data

/azp run libertem.libertem-data

Hi @AnandBaburajan, many many thanks for keeping working on this. :-) I had a look over the user interface, and it looks simple enough! 👍
A question regarding the parameter names: The new code uses consistently the parameter names sig_shape and nav_shape to specify dimensions, if I understood correctly? In the web GUI I saw code to handle the transition from detector_size and scan_size gracefully for type RAW. However, at least in https://github.com/LiberTEM/LiberTEM/pull/793/files#diff-715f7d221cdf059e6e5b75eb06440ffa77f43cb794a507d538d7c138bf6d06f2 it looks like a hard deprecation of the previous parameter names instead of the new ones for the Python API. This will break user code that uses our "official" API.
What about using the previously established terms detector_size and scan_size consistently instead of the new terms sig_shape and nav_shape, even if the old names are perhaps not optimal? In case we have compelling reasons to transition to the new names, we should keep the old ones as aliases and also confirm with a few unit tests that they still continue to work. We should only disable them if there is a compelling reason, i.e. if the trouble of keeping them is a lot larger than the trouble we create for users when breaking their code.

What about using the previously established terms detector_size and scan_size consistently instead of the new terms sig_shape and nav_shape, even if the old names are perhaps not optimal? In case we have compelling reasons to transition to the new names, we should keep the old ones as aliases and also confirm with a few unit tests that they still continue to work. We should only disable them if there is a compelling reason, i.e. if the trouble of keeping them is a lot larger than the trouble we create for users when breaking their code.

I think this project was a good opportunity to fix the names, although I do agree that the old names should keep working. The changes without deprecation warnings are, as far as I can see, limited to the web API, which we don't make stability guarantees for, yet.

It looks like just using detector_size and scan_size consistently will be a much smaller and simpler change, right? How strong is your feeling regarding the names? For me both are acceptable, so for me the amount of work and code complexity would be the deciding factor.

The changes without deprecation warnings are, as far as I can see, limited to the web API

Ah, now I see the code that handles the deprecation in https://github.com/LiberTEM/LiberTEM/blob/70d7fe2abc6573d5ea80919ac24092258119b8e7/src/libertem/io/dataset/raw.py
Ok, so if you think the new names are worth it, we can go for it!

It looks like just using detector_size and scan_size consistently will be a much smaller and simpler change, right? How strong is your feeling regarding the names? For me both are acceptable, so for me the amount of work and code complexity would be the deciding factor.

I was looking forward to the new parameter names, actually, so I'm in favor of the new names. In most formats, these parameters are new, so changing everything back to detector_size/scan_size would be actually more work right now, even if it makes the diff smaller for one or two formats.

We can just keep the old ones working indefinitely, right?

We can just keep the old ones working indefinitely, right?

True, as long as we keep them officially deprecated and remove them from the documentation, where they could confuse our  users. The only drawback is bloating up the __init__ method(s), but that should be limited to only a few data set implementations.

@AnandBaburajan the interactive frame skipping in the GUI is awesome, by the way! Just an idea for better usability: When I specify an invalid sig shape, I get an error message only after clicking "Load dataset", and I have to open again. Would it also be possible to show that error message interactively and grey out the the "Load dataset" button? For many datasets the sig size seems to be a constant, so if a sig shape is valid could be calculated even on the client if it knows the correct total size, right?

@AnandBaburajan regarding reshaping a RAW dataset I am honestly a bit confused with the user interface. Sig and nav are technically completely determined by the user, right? If I mess with the shapes, I sometimes get an error message and sometimes not. I honestly don't understand what is the determining factor of what works and what doesn't.
The error message should perhaps help the user to understand what is going on? invalid dataset: cannot reshape array of size<PHONE_NUMBER> into shape (66052,newaxis) leaves me a bit puzzled, and I don't know from that what I should do to make it work.

invalid dataset: cannot reshape array of size<PHONE_NUMBER> into shape (66052,newaxis)

@uellue It's due to incorrect calculation of image_count due to an invalid sig_shape. On the master branch, RAW's image_count was calculated using self._meta.shape.flatten_nav()[0] and no errors were thrown on incorrect detector_size. But to implement sync_offset, RAW needs to calculate the image_count properly taking into account filesize, sig_shape and dtype.

The error message should perhaps help the user to understand what is going on?

Since there's no way to get the native_sig_shape for RAW and since I need to calculate the image_count properly, I think the only error message I could provide is sig_shape is incorrect.

Are the ideas related or same as what @uellue suggested regarding the error messages?

No, I was mostly thinking about cosmetics / presentation / layout

invalid dataset: cannot reshape array of size<PHONE_NUMBER> into shape (66052,newaxis)

@uellue It's due to incorrect calculation of image_count due to an invalid sig_shape. On the master branch, RAW's image_count was calculated using self._meta.shape.flatten_nav()[0] and no errors were thrown on incorrect detector_size. But to implement sync_offset, RAW needs to calculate the image_count properly taking into account filesize, sig_shape and dtype.

The error message should perhaps help the user to understand what is going on?

Since there's no way to get the native_sig_shape for RAW and since I need to calculate the image_count properly, I think the only error message I could provide is sig_shape is incorrect.

What is actually the root cause? Is it that the total shape of the memory map is not divisible by the given sig_shape? In the spirit of #256, shouldn't the raw data set be able to handle something like a partial frame at the end gracefully (i.e. ignore any extra data at the end)? I'm thinking about opening files that are either corrupted, truncated or just weren't acquired completely (i.e. someone killed the process that writes the data, after half a frame was written)

I tried doing dynamic validation in Formik some time ago for sync_offset but I haven't figured out a solution yet.

I would prefer proper validation. As an implementation idea:
The withValidation helper could accept an additional optional customValidation property in the opts. This function takes (values, props) and returns an error list compatible with formik. Note that props has access to info which then needs to contain the np.prod(sig_shape) for those formats that have it. In the validate function defined in withValidation, the returned error list needs to be merged with whatever is returned by validateOpen, and the merged result needs to be thrown as an exception (merging and throwing could be extracted into helper functions, see convertErrors for how the error list needs to be structured. Now that I'm looking at it, it's actually a name → error map).
Then, in the parameter form component, where withValidation is applied, the formats can define a customValidation. In most cases, this will just be a function that checks np.prod(sig_shape) == info.sig_size or similar, and can be shared among these formats. For raw this is then different according to the discussion above.
What do you think about this approach, is the description clear enough?

@sk1p & @uellue thanks for the suggestions and ideas! I've made the changes, please let me know your suggestions.
While loading RAW in the GUI, the sync_offset info (frames skipped, inserted, etc) calculation and validation sync_offset would require knowledge of all the numpy dtypes and their corresponding itemsizes. For now, I've hidden the 'info' just for RAW. Is there any other solution except storing info of all the numpy dtypes in the client?

/azp run libertem.libertem-data

While loading RAW in the GUI, the sync_offset info (frames skipped, inserted, etc) calculation and validation sync_offset would require knowledge of all the numpy dtypes and their corresponding itemsizes. For now, I've hidden the 'info' just for RAW. Is there any other solution except storing info of all the numpy dtypes in the client?

Ah, I see what you mean. I think for now, hiding the "info"-part is fine. Handling the dtype itemsizes on the client can be error-prone, as you need to duplicate what numpy does, for example you would need to handle the different aliases for each dtype (for example: uint16, u2, <u2 etc.). An alternative could be to calculate this "info" via a server-side feedback, but I think this is something that can wait.
(As an aside, I do have some Ideas™ what could be done with such a feedback mechanism, for example, it could offer a small visual preview of a few frames of the data set with the current settings, so the user can see that their sync_offset looks good, or that the dtype/sig shape for a raw dataset is correct).

@AnandBaburajan thank you for the update! It works much better now and will be very useful. :-) Only a few little things, can also be cleaned up in a follow-up PR if it helps to merge this soon:

The message "must be of size: <...>" if the user enters a bad sig shape is a bit subtle and may not be visible at first glance or be identified as an error. Can it perhaps be colored red? One could also highlight the fields that should be changed to resolve this error with red color, if possible?
Since the info on blank and skipped frames is hidden for RAW, the unit of "Sync Offset" was unclear to me. I expected it to be in bytes and not frames when I used it for the first time. Can the unit be included in the GUI? This wouldn't hurt for the other formats as well, I guess?

Since the info on blank and skipped frames is hidden for RAW, the unit of "Sync Offset" was unclear to me. I expected it to be in bytes and not frames when I used it for the first time. Can the unit be included in the GUI? This wouldn't hurt for the other formats as well, I guess?

Agreed - wording can be improved. The other issue (validation msg styling) can be fixed in a separate PR IMHO, as it is orthogonal to this PR.

Since the info on blank and skipped frames is hidden for RAW, the unit of "Sync Offset" was unclear to me. I expected it to be in bytes and not frames when I used it for the first time. Can the unit be included in the GUI?

The message "must be of size: <...>" if the user enters a bad sig shape is a bit subtle and may not be visible at first glance or be identified as an error. Can it perhaps be colored red?

Done!

One could also highlight the fields that should be changed to resolve this error with red color, if possible?

I'll definitely look into that later.
Please let me know if you've any more suggestions, thanks! I'll also go through the whole code thoroughly to make sure everything is fine, any more tests I should write, etc. Regarding the TODO items, I do wish to complete atleast HDF5 (https://github.com/LiberTEM/LiberTEM/issues/441#issuecomment-735218240) in this PR. I think I can finish it by this weekend.

/azp run libertem.libertem-data

Would it be possible to perform these tests on a subset of data, without affecting the correctness?

Done! I've also removed some tests from some formats which essentially check a feature already tested in another format. For example, +ve sync_offset with ROI. For FRMS6, I'm hash checking some of those slow tests' results. Is that a good practice?

For HDF5, keep in mind that there is a chunked variant that can't be read directly via mmap! In my opinion, in that case, it makes sense to fall back to the slower "per-frame" method that is currently used if a roi is set.

I'm not sure about going with the "per-frame" method for cases except a roi because I found it to be very slow. I should have shared the benchmark results earlier, sorry. Now I think it would be better if this is done in another PR: since the indexing of chunked data is abstracted away by h5py, I think I should go deeper to find a solution using h5py, slicing and read_direct to properly read both normal and chunked data, with/without reshaping/sync_offset.
My current solution also lacks this (just for HDF5):

On the Python side of things, what we haven't fully discussed is how missing data (#256) is handled from a UDF point of view. One thing I noticed is that if we keep it like this, UDFPreprocessMixin.preprocess doesn't have a chance to initialize result buffers at the positions where the data is missing, which may be a problem (although I don't think we have a user of preprocess yet, at least not as part of LiberTEM). The code could be changed such that the partitions do span the missing data, i.e. include those positions in their slice, but don't yield tiles/frames for them

/azp run libertem.libertem-data

Done! I've also removed some tests from some formats which essentially check a feature already tested in another format. For example, +ve sync_offset with ROI.

That's okay, although generally, "redundant" tests are also okay, as long as they test something the other tests can't test, for example the integration of the sync_offset feature into a particular DataSet implementation.

For FRMS6, I'm hash checking some of those slow tests' results. Is that a good practice?

This is fine as long as it's clear from the context how to derive the real unhashed expected value. Maybe you can add the exact command to generate the hash as a comment?

I'm not sure about going with the "per-frame" method for cases except a roi because I found it to be very slow. I should have shared the benchmark results earlier, sorry. Now I think it would be better if this is done in another PR: since the indexing of chunked data is abstracted away by h5py, I think I should go deeper to find a solution using h5py, slicing and read_direct to properly read both normal and chunked data, with/without reshaping/sync_offset.

Separate PR sounds good. HDF5 + roi will soon become a more important use case (together with #764), hopefully in most cases the files won't be chunked and can be read directly, via mmap or similar.

Looking good! The test failures are possibly related to #921, when that is merged, maybe it's a good idea to rebase to master.
Btw. if rebasing the 73 commits is too error-prone, you can alternatively squash them all into one commit with an interactive rebase, and then only rebase that one commit.

:+1:

Let me know when you think this no longer WIP, from my side, this looks good to go!

I'll have another look and let you know, thanks! Also, is there a particular reason why MRC isn't allowed in the GUI yet?

is there a particular reason why MRC isn't allowed in the GUI yet?

I don't think there is a reason, should be easy enough to add.
Btw. the failing CI is because of the very fresh sphinxcontrib-bibtex release, which needs a bit of configuration. I've opened #924 for this.

/azp run libertem.libertem-data

Let me know when you think this no longer WIP, from my side, this looks good to go!

@sk1p & @uellue I think this can be merged now :D

This is great, congrats and many thanks @AnandBaburajan!

Congratulations @AnandBaburajan and many many thanks for persevering!  This is really a major improvement. :-)

Congratulations from my side too!
Thank you very much for all your efforts!

Great work @AnandBaburajan :confetti_ball: :tada:


Can't parse symbols according to spec. More powerful "parse_edn" pattern matching?

According to https://github.com/edn-format/edn#symbols the following is valid (and confirmed with clojure.edn/read-string).
(clojure.edn/read-string "-foobar")
-foobar
(type (clojure.edn/read-string "-foobar"))
clojure.lang.Symbol

Currently parse_edn is only matching on the first character, which is insufficient for this case. -foobar gets treated as a Number.
https://github.com/naomijub/edn-rs/blob/8353fc4cbfb1938849ffed0a4a53951b95706f24/src/deserialize/parse.rs#L41
I believe it should be possible to be passing around a &str instead of &mut std::iter::Enumerate<std::str::Chars>, which would allow for str pattern matching. I don't know if this is a slippery slope eventually recreating regex. Before a big change, a discussion doesn't seem like a bad idea. There might be an even easier way or additional considerations.
Rational:

New edge case
It could help clean up #103
If this was done, all the very expensive and heap allocating clone()s on the Enumerate could be removed.
None of this is externally pub, so no other projects would break. I believe it would technically be possible to even have the return not be an owned String but str slices from the original &str too. This would make it so there's no heap allocations, but would also break existing code. Perhaps worth considering before 1.0.0.

On this same topic, here's another case where more powerful pattern matching is going to be required
#[test]
fn quoted_symbol() {
    let mut edn = "('(symbol))".chars().enumerate();
    let res = parse(edn.next(), &mut edn).unwrap();

    assert_eq!(
        res,
        Edn::List(List::new(vec![
            Edn::Symbol("'".to_string()),
            Edn::List(List::new(vec![Edn::Symbol("symbol".to_string()),]))
        ]))
    );
}

thread 'deserialize::parse::test::quoted_symbol' panicked at 'assertion failed: `(left == right)`
  left: `List(List([Symbol("'(symbol")]))`,
 right: `List(List([Symbol("'"), List(List([Symbol("symbol")]))]))`',```

expected result
(clojure.edn/read-string "('(symbol))")
(' (symbol))

The core issue of symbols not being parsed according to spec hasn't been resolved with the other resolved issues/PR. In particular something like (apply + '(1 2 3)) gets parsed as List([Symbol("apply"), Symbol("+"), Symbol("'(1"), UInt(2), UInt(3)]))
Do you have any thoughts about not using Enumerate for better pattern patching and removal of all the clone()s in parse.rs? Taking a look at it, even though parse is pub, the module is not
https://github.com/naomijub/edn-rs/blob/8353fc4cbfb1938849ffed0a4a53951b95706f24/src/lib.rs#L43
so we could change things internally without breaking anything. Maybe I'm missing something, but it would be nice to look into the next characters without having to allocate a new string/iterator.

I was actually able to do this without refactoring the Enumerate, but I still think it could be a good idea.

I don't know how much performance is a concern (I assume some, because benchmarks are advertised), but on top of making matching a lot cleaner, there's huge performance and memory usage gains by not using Enumerate.
https://github.com/naomijub/edn-rs/blob/bec6ea2d2b2613b9189366e30ee1fa43b78103c8/examples/edn_from_str.rs
right now this example uses 52 heap allocations (6098 bytes in total).
changing it to
let edn_str = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.";

ends up being 231 allocations over 18,995 bytes. Each clone has to deep copy the entire String.
@naomijub was there a reason for Enumerate? Compared to json, I think EDN probably has a lot more concerns, but it's possible to do no_std (0 allocations). I don't think you can do EDN without escaping so no allocations is likely not possible. Using the same interface of returning owned Strings, I think the Lorem example would be only 70 allocations (~500 bytes?) using references internally. Looking at the competition, something like #[serde(borrow)], could even potentially go down to 1 allocation for the Vec.
This is beyond the recent mostly trivial PRs. If you're not against it, I'll take a poke at it later.


Confusion about "optional" flag

Hi,
I'm trying use the "optional" flag, but it doesn't seem to be working the way I expected it to. What I'd like to do is have a flag, "password", that can be given either with or without arguments. (we need to support an empty --password argument for backwards compatibility)
That is, I'd like
program --password --otherOption
and
program --password "mypassword" --otherOption
to both work.
Right now I have
Password  string `short:"p" long:"password" optional:"yes" description:"password for authentication"`
in my options struct. However when I do
program --password "mypassword" --otherOption
the argument to --password is not actually captured, and so "mypassword" ends up in the array of leftover arguments.
Am I misunderstanding what "optional" is for?

Yes I agree, this is a bug.
package main

import (
	"fmt"

	"github.com/jessevdk/go-flags"
)

func main() {
	var opts struct {
		Foo string `long:"foo" optional:"true" optional-value:"no argument"`
		Bar bool   `long:"bar"`
	}

	args, err := flags.Parse(&opts)
	if err != nil {
		fmt.Printf("Error: %v\n", err)
	}

	fmt.Printf("Args: %#+v\n", args)
	fmt.Printf("Opts: %#+v\n", opts)
}

$ go run tt.go --foo --bar
Args: []string{}
Opts: struct { Foo string "long:\"foo\" optional:\"true\" optional-value:\"no argument\""; Bar bool "long:\"bar\"" }{Foo:"no argument", Bar:true}
$ go run tt.go --foo baaaaaa --bar
Args: []string{"baaaaaa"}
Opts: struct { Foo string "long:\"foo\" optional:\"true\" optional-value:\"no argument\""; Bar bool "long:\"bar\"" }{Foo:"no argument", Bar:true}

@jessevdk can you confirm that in the last execution "Foo" should use the given argument?

This is explicit behavior, as per:
https://github.com/jessevdk/go-flags/commit/837238c985d634229928667370802a7a75cba652
This makes long arguments consistent with short arguments. We have been down this road before (can't find the specific issue), and I'm not convinced we should change this behavior (again).

I definitely understand where you're coming from re: consistency, but my team doesn't find the current behavior intuitive given the label of "optional." Given how longstanding this behavior is, I can't really argue that it should be changed, so I have two propositions for you:

Let's make the documentation clearer so that others don't misunderstand "optional" again
Add another tag or sentinel value for the "optional" tag (ie optional:"noequals" or something) in order to change the parser behavior to be what I've described above. This is an awesome library and we'd love to be able to peg our dependencies on this repository instead of a hacked fork of it.

The issue is not just consistency. The real issue is that with optional arguments as you propose, you simply can't distinguish between an argument being an optional value, or a rest (positional) argument.

Yes!
I'm not convinced that it's worth making this more complex. The bar to adding more options/tags is getting higher (there are already too many as it is). Moreover, this will change the behavior so it's no longer standard (how do users know about this).

I've improved the documentation, but I don't think we should change the behavior.


refactor(frontend): ic-token.validation -> schema

Motivation
As discussed offline in the context of #3357. We actually adopt a new convention in which Zod schema related are provided in sub-folder schema.
This prevents the introduction of circular dependencies when the validation uses both types and Zod schema.
Changes

Rename ic-token.validation to ic-token.schema

Relates to https://github.com/dfinity/oisy-wallet/issues/2883


Multiple calls to glfwMakeContextCurrent on Linux crash

Hi,
If I modify the Threads demo (https://raw.githubusercontent.com/LWJGL/lwjgl3/master/modules/core/src/test/java/org/lwjgl/demo/glfw/Threads.java) so that
glfwMakeContextCurrent(window);
is added right after
glfwShowWindow(window);
the demo crashes with:
Exception in thread "Thread-0" java.lang.NullPointerException
at org.lwjgl.system.Checks.checkPointer(Checks.java:100)
at org.lwjgl.opengl.GL.createCapabilities(GL.java:273)
at org.lwjgl.opengl.GL.createCapabilities(GL.java:225)
under Linux (Ubuntu, Nvidia proprietary drivers)
It seems to work OK on a Mac.
I thought it should be possible to pass the ownership of the OpenGL context between threads, but maybe I am missing something.

Yes, it is possible to pass the ownership of an OpenGL context between threads. However, you must detach the OpenGL context from a thread before making it current in another thread.
Read the documentation of GLFW's glfwMakeContextCurrent.
So before making the context current in thread B you first have to call glfwMakeContextCurrent(0L) in thread A.

Thank you very much for the info. I have read the documentation but it is not obvious you must detach the context first.


Allow to manage multiple connectors when it apply

Fixes #107
This is a base of discussion about the best way to manage multiples connectors in one command
For example, I choose to add a --all option to process all connectors. But it could be a good idea to allow regexp or wildcard pattern instead (or both)
For apply, I choose to not allow -n with multiple -f because it can be tricky to use otherwise. It might be too restrictive.
Just let me know if it's going the good way. I can adapt the implementation following recommendations.
And then apply the changes on all the other commands where it applies.

for the regexp stuff : I was thinking about a -e option that would then treat the names as regexp filter on all connectors names instead of --name-pattern=.*.
what do you think ?

I'm facing difficulties generating the completion. Picocli seems to (currently) require an empty constructor. Which is not compatible with the constructror injection pattern used on some command

Picocli seems to (currently) requires an empty constructor. Which is not compatible with the constructror injection pattern used on some commands

Yes, that's true. I thought I'd have added those default constructors to all the commands, but I may have missed some. Just add them in this case. They'll only be used for the completion generation, otherwise the inject-ed constructors are used.

for the regexp stuff : I was thinking about a -e option that would then treat the names as regexp filters on all connectors names (like grep) instead of --name-pattern=.*.

So would -e take the pattern then (which I think grep is doing)? Or would -e be a "switch" that impacts how the --name value is interpreted? Can you share the entire command invocation perhaps?

for the regexp stuff : I was thinking about a -e option that would then treat the names as regexp filters on all connectors names (like grep) instead of --name-pattern=.*.

So would -e take the pattern then (which I think grep is doing)? Or would -e be a "switch" that impacts how the --name value is interpreted? Can you share the entire command invocation perhaps?

Good point : Not exactly like grep then. I see it more like a switch ;
kcctl pause connector -e 'topic\.\d' 'othertopic\.\d'
vs
kcctl pause connector topic.1 topic.2 othertopic.1 othertopic.2 ...
It could be confusing as the user may consider the -e as a tag for the name just after. But I feel like it's more compliant with the way the commands are currently designed.
tell me your preference :

--name-pattern before 'normal' names : then we have to decide how to manage mix of --name-pattern and 'normal' names argument
-e like grep : might be tricky to manage commands like  -e 'reg1' 'name1' -e 'reg2' 'name2'
-e like a switch

Picocli seems to (currently) requires an empty constructor. Which is not compatible with the constructror injection pattern used on some commands

Yes, that's true. I thought I'd have added those default constructors to all the commands, but I may have missed some. Just add them in this case. They'll only be used for the completion generation, otherwise the inject-ed constructors are used.

I'll do

I made a commit taking in account your feedback. Currently it is a switch -e on pause, but I can change to something else.
I redesign the patch for multiple files on Apply. I think it is more robust like this, but it involve more changes in the code that I was expecting. So don't hesitate to tell me if you're not OK with some of my changes. I can adapt.
I'll apply the changes on others commands once we are OK with this ones

Currently it is a switch -e on pause, but I can change to something else.

Cool, I think that's a good approach. I wa leaning towards --name-pattern initially, since I (incorrectly) assumed that the connector name would currently be given via --name. But instead it's just given after the command (i.e. a parameter and not an option in PicoCli terms). So having an option like --regexp which controls how the parameter is interpreted makes sense 👍 .
My only question would be why "-e"? I.e. what does "e" it stand for, "expression"? How about using "-r"/"--reg-exp" for easier memorization? Btw. on "--reg-exp" vs. "--regexp", it probably should be the former, as we use the  dash in multi-word option names (e.g. see SetContextCommand).

I redesign the patch for multiple files on Apply. I think it is more robust like this, but it involve more changes in the code that I was expecting. So don't hesitate to tell me if you're not OK with some of my changes. I can adapt.

Thanks, it all LGTM and I think we've achieved agreement on the way forward. Apart from bringing regexp support to all commands taking a connector or task name, one other thing to do would be updating the list of commands in the README (it's the out of kcctl without any commands).

Patch already have a -r option

I currently setup -r and --reg-exp on all commands except Patch who is -e --reg-exp
probably not the best solution
Let me know how you want to deal with the -r short option conflict

Patch already have a -r option

Ah yes, I see. So looks like we should use -e / --reg-exp consistently for all commands then. Using different short names for the same option for different commands is too inconsistent and confusing.

Interesting, I haven't seen that one before. Are you using Java 17 to build
kcctl=

Message ID: @.***>

Yes java 17. It works without my commit.
I'll dig then

It's related to the ApplyConnector record I added in ApplyCommand.
Graalvm doesn't seem to like when it is part of a class field type
So I changed the applyConnectors type from ApplyConnector[] to List (changing the visibility of the record doesn't help. List<ApplyConnector> neither)
It's a little bit uggly as it force to cast. But it should do the job
So : I think we're good...
Tell me if you see anything

Hey @jvenant, so this LGTM, merging. Thanks a lot for this very nice piece of work!


Restarting the emulator changes the default flow account address and causes friction with FCL

When using flow dev , if we restart the emulator, the flow address set to the default account always changes. That's not a problem from the emulator perspective since we can use the shorthand import, but when using FCL, I need change the value in it's configuration to match the new one, which is annoying.
I can see two possible solutions:

Can we pin the default address so we have a deterministic value whenever the emulator restarts?

Are we thinking in have a canonical import resolution for the emulator and FCL? It would be great if we can do something like we are doing for the emulator in FCL Tx's as well.

# Emulator
import "MyNFT"

# FCL
import MyNFT from 0xMyNFT

I know I can also solve that problem by not using super commands at all, and own my flow.json.
That's my main issue with super commands so far, it has been hard to couple it with manual changes that I want to do in my flow.json file.

This is being addressed as part of FCL improvement with PR WIP https://github.com/onflow/fcl-js/pull/1511


Figure out Windows/Ubuntu discrepancy for test_get_nearest_family given a page with children

Jump to line comment mentioned above


Extending hygiene tests set - no unused imports

Signed-off-by: Pawel Garbacz<EMAIL_ADDRESS>Description
This test is to find unused imports in the whole (aggregated) FIBO ontology.
Fixes: #1081
Checklist:

[X] I'm familiar with the FIBO developer quide. My contribution meets all the requirements described there.
[X] My contribution follows the principles of best practices for FIBO.
[X] My changes have been reconciled with latest master and no merge conflicts remain.
[X] This PR is related to exactly one issue. The issue is referenced by using a GitHub keyword such as "fixes", "closes", or "resolves".
[X] Hygiene tests have been applied by a PR with "(WIP)" in title.
[X] The issue has been tested locally using a reasoner (for ontology changes).

@rivettp
As for:

Hmm I don't see where the triple ?s ?p ?o is linked to the ?importingOntology. Which would actually be hard to do unless you use the quad representation which has a named graph for each ontology.

I can add the following clause to FILTER NOT EXISTS filter:
FILTER (CONTAINS(str(?s), str(?importingOntology)) || CONTAINS(str(?p), str(?importingOntology)) || CONTAINS(str(?o), str(?importingOntology)))

@rivettp
As for:

There could be some false matches here (which will be considered a usage whereas it's not) e.g. if one ontology has a URI which is equal to another with an extra word or two added.
Also if a URI is contained in the string that is the object of a datatype or annotation property.

If I get you right, the following change may a remedy.
Instead of:
FILTER (CONTAINS(CONCAT(str(?s), str(?p), str(?o)),str(?importedOntology))).
we could have:
FILTER (REGEX(str(?s), CONCAT(str(?importedOntology),".+")) || REGEX(str(?p), CONCAT(str(?importedOntology),".+")) || REGEX(str(?o), CONCAT(str(?importedOntology),".+")))

One thing I am wondering about - there is at least one case where I've explicitly imported an ontology in the latest revisions to loans because that ontology adds restrictions on DebtInstrument that we need for a loan, but there are no direct references to anything.  Would this require that I add some reference to the SEC/Debt/DebtInstruments ontology in the importing ontology to keep that import statement?

@ElisaKendall
Which ontology did you mean?