Conversation
…ot needed. Added HAVE_ADIOS2 and HAVE_MPI as two separate variables in a cmake file as ADIOS can be use without MPI
|
It is interesting why it didn't attempt to run CI. Last time I did anything with casarest it ran the CI. Although with this particular PR it won't change much as we don't have CI with ADIOS setup, so it would just be checking building without ADIOS. |
|
I think it's because the PR was created by the user |
|
@rtobar Do you know how to trigger the CI manually? I only seem to have buttons to merge. And while we're at it, do you have any problem with this code made part of casarest, at least for the time being? This is essentially the original (I believe Alex's) code which we decided to move from yandasoft code base to casarest because it has casacore's image interface (and looks like a casacore class). My intention is only to check that things build without breaking any non-ADIOS stuff but I won't comment on any implementation or design things of this code. |
|
Ah, it looks like I found it - needed to click "Approve and run" on another tab. |
|
I think CI failed just because of this. @vuo006 can you make the changes? |
…if casacore is not built with ADIOS2. Also, added #ifdef guard in ADIOSImage class so that it is only implemented and compiled if ADIOS2 is built with CASACORE
|
I verified that casarest builds against casacore-3.7.1 on my machine without ADIOS (although CI also verified it). There are a few minor comments about type passed to the methods (e.g. needed to be by const reference, rather than by copy and table row type) which was like that since the time we tried to integrate the code into yandasoft. I thought I fixed them there, but apparently I didn't. More serious comment is about reverting my change with unique_ptr. This may have introduced a memory leak. Also, it may be worth to add a couple of lines to README.md mentioning that to build with ADIOS2 one has to define the corresponding cmake variable and build casacore with ADIOS2 as well. Apart from this I'm happy. @rtobar do you have any other suggestions? |
|
@VoronkovMA honestly I'm not very familiar with the Image interface this class is based on, so I can't really comment much on that front. The setup of the storage manager looks roughly correct, although a bit hardcoded at times (all those mgard/accuracy parameters). I assume that's something you guys are comfortable with, and there's also the |
|
@rtobar yes, I guess we're happy with the current state of things for now. The idea is to park this code in casarest until someone (probably outside of ASKAP team) is willing to polish it and move to casacore. It doesn't seem to be a good fit for ASKAPsoft code base anyway (+ may be useful for other people outside of the scope of ASKAP). |
|
@vuo006 unfortunately, a few more places need a minor patch - some lines were overlooked, when you changed types. Perhaps also comment in here, may be then github would allow you see my review comments. |
|
I have made the changes as indicated. Hope I did not miss any. |
|
I think a couple of places were missed, although I thought I went through the code with a fine comb. |
|
I made the changes to the latest feedback. I still cant see the affected code in GitHub though |
Add a new ADIOS Image class to casarest