mhardcastle / ddf-pipeline

LOFAR pipeline using killms/ddfacet
GNU General Public License v2.0
24 stars 20 forks source link

Comparison between new DDF/kMS and old DDF/kMS #337

Open twshimwell opened 1 year ago

twshimwell commented 1 year ago

This issue is for the final checks of the new DDF/kMS before switching LoTSS processing to use the new rather than old versions.

6 LoTSS fields (P030+51, P054+36, P216+15, P269+63, P316+81 and P347+08) are being processed using the same parset and facet layout with the new and old versions of DDF/kMS.

I shall update this ticket as the fields finish processing but the new maps are stored in:

/beegfs/car/shimwell/Cyril-tests/FINAL-TESTS/P???+??

Whereas the old ones are in:

/beegfs/car/shimwell/Cyril-tests/FINAL-TESTS/comparison-maps/P???+??

twshimwell commented 10 months ago

Ok 5/6 fields now fully finished for the Stokes I part. Comparing the image_full_ampphase_di_m.NS.app.restored.fits maps:

P030+51 - very comparable. Possibly a new sofware is a tiny little fraction worse (increase in rms by 5%). P090+77 - almost identical. Possibly new software a tiny fraction better. P050+26 - almost identical P269+63 - almost identical P316+81 - very comparable. Possibly new software fractionally better.

So I think things are looking good.

I should probably do some source finding on the maps to compare source properties too.

@cyriltasse did you have thoughts on the QU cube mapping? Do we need to change to one of the accepted modes or can DDFacet do just QU again?

mhardcastle commented 10 months ago

Presumably we could just do IQU (but would then need to change the way we build the cubes).

How is the speed looking wrt the old software?

twshimwell commented 10 months ago

ive not taken a look at the speed comparisons yet but should do that too. Its all stored in the summary file at the end of ddf pipeline is it?

Yeh we could indeed just do IQU. I guess this is slower though too.

mhardcastle commented 10 months ago

Timings live in the log files but you should be able to run analyse_logs.py to get them out (expects to open a matplotlib window).

bennahugo commented 10 months ago

Stems from needing to create a valid PSF (presuming the HV phase is calibrated further up). IQU will create a stokes I psf and clean QU with said PSF (in Hogbom Pratley &. Johnston-Hollitt CLEAN). The QU mode was buggy -- it doesn't form a valid PSF to clean with. @cyriltasse

I guess you can add back QU mode in the gridder and degridder but you will need to feed in a valid PSF elsewhere.

cyriltasse commented 10 months ago

Thanks for the clarification, makes a lot of sense! We should maybe put a visibility matrix giving IQUV=1111, but the fix will work for now I guess

twshimwell commented 10 months ago

OK well for the test runs I changed things to using IQU. The polarisation imaging now runs completely fine. So just need to make some edits to the pipeline to remove the I from the IQU. I'll try to do this today if I get a chance.

For the low QU cubes they increase in size from about 18GB to 25GB per observation (so about 23TB for the entire LoTSS). But anyway the I isnt much use unless we were to deconvolve it to unless we do that there isn't much point keeping it.

mhardcastle commented 10 months ago

Better to keep the data products consistent as well.

twshimwell commented 10 months ago

ok this is pull request https://github.com/mhardcastle/ddf-pipeline/pull/343.

I believe fine with old software too because IQU is still possible there.

twshimwell commented 10 months ago

More quantitative comparison. Here is the number of sources in the old software and new software runs

Field, Num sources old, Num sources new, Num sources old / Num sources new P030+51, 7003, 7088, 0.988 P050+26, 10528, 10399, 1.012 P090+77, 6135, 6172, 0.994 P269+63, 9516, 9582, 0.993 P316+81, 5291, 5430, 0.974

So it varies a bit but seems that generally we do very slightly better with the new software in the final image_full_ampphase_m.NS images.

twshimwell commented 10 months ago

Flux density scale comparison shows they are within a few % (ranging from 1-4)

bennahugo commented 10 months ago

My question is what is being tested here? Release versions or branches?

Just a note to proceed with caution with the development branch in any serious science -- it still has significant issues, e.g. the restoring beam is convolved onto the image with the wrong PA applied: https://github.com/cyriltasse/DDFacet/pull/863#issuecomment-1846803119

mhardcastle commented 10 months ago

We've been using development branches for serious science since 2017 I'm afraid... but I agree that the restoring beam issue needs looking into!

twshimwell commented 10 months ago

Do you have the names of the branches we are moving between on DDFacet and kMS? I have forgotten.

mhardcastle commented 10 months ago

Our current processing is actually based on master.

Cyril's dev branches seem to be

  export BCH_DDF=UseAPP_kMS_MeerKLASS
  export BCH_KMS=APP_Predict_Compress_PolSmooth_HybridSM_OpFit
mhardcastle commented 10 months ago

(but that DDF version looks like it is a branch off the one that had the issues that @bennahugo is referring to, so that's a potential significant issue)

cyriltasse commented 10 months ago

We manually set the restoring beam to something circular so probably not an issue for us but yeah - to be fixed indeed (it's a very minor fix)... @bennahugo do you see anything else obvious? I'll start running the current version through the unitary tests asap

mhardcastle commented 10 months ago

We use an elliptical beam at low dec, Cyril. So we need this to be fixed... hard to see how it could have got broken!

cyriltasse commented 10 months ago

Oh ok - I'll fix it next week then!

hard to see how it could have got broken!

It got broken because for the rectangular pixels/images I've had to swap/invert a lot of things in the grids and facets, so that does not completely surprise me

mhardcastle commented 10 months ago

Ahh that makes sense. Of course at really low dec we could be using rectangular images and pixels for better results -- but maybe that's a step too far.

bennahugo commented 10 months ago

Happy for you to run them through the tests again. Remember there is also an issue with applying old kms solutions so we need to put in database versioning of sorts. I would say you should just pickle the version and if it doesn't exist it falls back to older interpolation.

On Fri, Jan 19, 2024, 12:59 Martin Hardcastle @.***> wrote:

Ahh that makes sense. Of course at really low dec we could be using rectangular images and pixels for better results -- but maybe that's a step too far.

— Reply to this email directly, view it on GitHub https://github.com/mhardcastle/ddf-pipeline/issues/337#issuecomment-1900192797, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4RE6VKRPPIDFGXFWVGSLLYPJGY7AVCNFSM6AAAAAA7JG67A2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBQGE4TENZZG4 . You are receiving this because you were mentioned.Message ID: @.***>

cyriltasse commented 10 months ago

@bennahugo I was looking at mu code and how the PSF is fitted and rendered to the restored and it seems all fine. The problem you were mentioning is only when you give manually defined beam?

bennahugo commented 10 months ago

No - not specifying a manually defined beam - have a look at TestMSMF and TestSSSF difference maps compared to master

twshimwell commented 8 months ago

Hey @cyriltasse @bennahugo just wondering if there was an update on merging Cyril's development branches of kMS/DDFacet into master (just watching emails fly by from the DDFacet GitHub about preparing for a v0.8.0 DDFacet release)

bennahugo commented 8 months ago

Not sure if @cyriltasse has made fixes yet - I know he has been working on his paper. There were a few issues including the beam orientation and the killms solutions not being backwards compatible with solutions derived from older versions of killms when applied in the imager (due to changes in the interpolation axis). I think those needs to be fixed before we can put it into a release version.

I would like to draft a release of the tested master branches so that we can put it into stimela v2 per discussion with @o-smirnov.

cyriltasse commented 8 months ago

I couldn't reproduce the error... I will have another try