[WIP] Support distributed read for other acquisition files #30
Conversation
tristpinsm
force-pushed
the
tpm/dist_read
branch
from
b4e539b to
67d2cd5
Compare
tristpinsm
force-pushed
the
tpm/dist_read
branch
from
67d2cd5 to
da02011
Compare
|
the other part of the unicode index map fix is in radiocosmology/caput#201 |
|
Ah interesting. I was facing similar issues with the HFB code. In the end I opted to go a slightly different route and write a draco style container that could read the archived HFB code (chime-experiment/ch_pipeline#99). I figured I could probably do the same for many of the other formats to (e.g. CorrData). I'll have a look at this later. It is a massive pain to need to be maintaining multiple implementations for distributed IO, so we should see if there are reasons to keep it separate, and if we can combine any of it together. |
Yeah I thought about taking this approach, but decided I didn't want to tackle changing the whole framework, especially because the
I agree that it makes no sense the way the code is organised at the moment, and it would be much better if everything used a common approach. It might be painful to try and move things over seamlessly and support all the old formats, but just making containers for the current acquisition formats would be an improvement. |
ch_util/andata.py
Outdated
| **kwargs, | ||
| ) | ||
|
|
||
| # Datasets that we should convert into distribute ones |
There was a problem hiding this comment.
If this routine is moving to BaseData maybe this list should be a class attribute so it can easily be modified by derived classes.
|
|
||
| Examples | ||
| -------- | ||
| Examples are analogous to those of :meth:`CorrData.from_acq_h5`. | ||
|
|
||
| """ | ||
|
|
||
| if distributed: |
There was a problem hiding this comment.
I kind of think classes should need to opt in to distributed support. See comments below for why,
| datasets=None, | ||
| out_group=None, | ||
| **kwargs, | ||
| ): |
There was a problem hiding this comment.
I think for the sake of people maintaining the module there should be a bit more clarity about what these various base methods are for. One thing that would help would be calling this method something else, e.g. _from_acq_h5_single, or somesuch that makes it clearer how this differs. It might also be good to put some extensive comments explaining the relationship between everything.
ch_util/andata.py
Outdated
| comm = MPI.COMM_WORLD | ||
|
|
||
| # Determine the total number of frequencies | ||
| nfreq = None |
There was a problem hiding this comment.
This seems problematic as not all datasets have a frequency axis, e.g. doing WeatherData.from_acq_h5(..., distributed=True) and I think this would crash on you complaining about the absence of a frequency axis.
There was a problem hiding this comment.
One option would be to allow subclasses to name an axis to distributed the read over. If it's present this routine uses it, if it's not present then you can't do a distributed read and it exits elegantly.
tristpinsm
force-pushed
the
tpm/dist_read
branch
2 times, most recently
from
f0b982c to
4f94fec
Compare
tristpinsm
force-pushed
the
tpm/dist_read
branch
from
4f94fec to
a97625c
Compare
|
ok, I think I've addressed your comments @jrs65 (thanks!). It feels like making this work is a hack, so I'm not sure what the right path forward is. I guess getting all of the |
tristpinsm
changed the title
|
It's not clear this is the way forward to enable distributed reading of other acquisition files. There is a similar problem with the HFB files, so we can shelf this for now and see how the HFB approach pans out and consider generalising it to all non-visibility acquisition files. |
I made these changes in order to be able to read
CalibrationGainDatafiles into distributed containers.