transientskp / tkp

A transients-discovery pipeline for astronomical image-based surveys
http://docs.transientskp.org/
BSD 2-Clause "Simplified" License
19 stars 15 forks source link

Different AARTFAAC frequency bands are being incorrectly treated as a single frequency band in TraP #492

Closed AntoniaR closed 8 years ago

AntoniaR commented 8 years ago

The two AARTFAAC frequency bands are currently being treated as a single frequency in TraP, which is causing issues when trying to compare the results from two different frequency bands and will affect the variability statistics. For instance this running catalogue source contains data from two different frequencies but only labelled as having one frequency band of 5.700e+07 Hz: http://banana.transientskp.org/master/vlo_SB002cendes/runningcatalog/9613/ However, from checking the images page, the images do have the correct frequencies and bandwidths.

I strongly suspect that this is related to how we define frequency bands and may require careful thinking/checking so as not to break TraP for other datasets.

I think we should only count observations as being from the same frequency if the mid frequencies lie within the bandwidth of the observations. i.e.

If TraP is already doing something like this, then we should check that the AARTFAAC image bandwidths and frequencies are being parsed correctly when building the running catalogue lightcurves.

gijzelaerr commented 8 years ago

I can confirm this is an issue with TraP. The database stores the frequencies information, but TraP buts them in the same band group. This is probably a problem with the getBand SQL code:

https://github.com/transientskp/tkp/blob/master/tkp/db/sql/statements/functions/getBand.sql

SB002cendes=> select DISTINCT band from image;
 band
------
   23
(1 row)

SB002cendes=> select DISTINCT freq_eff from image;
 freq_eff
----------
 57319640
 57421876
(2 rows)

SB002cendes=> select DISTINCT freq_bw from image;
    freq_bw
---------------
 192260.742156
(1 row)

SB002cendes=>
gijzelaerr commented 8 years ago

It looks like TraP is doing this by design, it rounds the frequencies to MHz:

https://github.com/transientskp/tkp/blob/a3b89ce432bb72c973757ea99dabc9051af02a41/tkp/db/sql/statements/functions/insertImage.sql#L115

Question is why this was originally a good idea, and why this changed. @bartscheers what do you think?

jdswinbank commented 8 years ago

See 8eb5f29f51e4e232e7a38dd23997de8c4097ce4c and discussion at Redmine #4801 (do you still have access to that? I don't.)

As I recall, bands are defined by a central frequency and bandwidth. Since you might expect these to wander a bit (due to flagging, or simply observing with nearly-but-not-quite the same settings as before), we decided to bin at the 1 MHz level so that observations at nearly the same frequency would appear in the same band. That's appropriate for the sort of observations being done with LOFAR (or, rather, that were being done with LOFAR a couple of years ago), but may not be for AARTFAAC.

@AntoniaR's suggestion is a plausible one, except that it probably breaks down at large fractional bandwidths -- if you observe with a 30 MHz bandwidth centred on 30 MHz and then again centred at 60 MHz, do you really want them treated as the same band? Maybe you could impose a maximum-bandwidth limit, or something.

Alternatively, if you intend TraP to be used for diverse instrumentation and science cases, it may be more appropriate to either adopt a per-instrument definition of bands or even make the user select them manually at database creation time.

gijzelaerr commented 8 years ago

thanks for you feedback John!

AntoniaR commented 8 years ago

Thanks from me too! Very useful and I hadn't thought of the large fractional bandwidths problem... I personally like the idea of a maximum bandwidth or fractional limit. The per-instrument or per-database definition could be something we have to go to in the future but I think it would be nice to avoid if possible.

timstaley commented 8 years ago

Reassigning milestone / Bumping this down the road a bit, since I'd like to get 3.0.1 out the door.

AntoniaR commented 8 years ago

This is a high priority issue for the aartfaac team... Why can't we get this issue into 3.0.1?

timstaley commented 8 years ago

Hey Antonia - literally about to push the 3.1 (nee 3.0.1) release. However, it sounds like this is the most pressing TraP issue, so we can make it top of the list - no reason 3.2 needs to take another 6 months.

AntoniaR commented 8 years ago

What is in 3.1 that is urgent for it to be pushed out? As far as I am aware from using it, 3.0 works fine and no major issues apart from this one...

timstaley commented 8 years ago

Well, there's a fairly serious bug regarding forced-fits (#496). It's unlikely to affect anyone not reducing mosaic images, but if you are then it's quite crucial (as we found out the hard way). I wanted to get this included in a proper version-numbered release so it's easy to check which version is in use.

There are also logging improvements which should be generally helpful, and specifically in diagnosing issues like #501.

AntoniaR commented 8 years ago

Sure. However, in future can we please be more informed about releases. For instance sending an email around the team outlining the plans for each release and when you plan to release it. This issue has been a high priority issue for the AARTFAAC team for 3 months and hence I think it should have been included in this release too.

gijzelaerr commented 8 years ago

The redmine url for ASTRON changed, ticket can now be found here https://support.astron.nl/lofar_issuetracker/issues/4801

gijzelaerr commented 8 years ago

So if I understand correctly currently the code:

So if I understand correctly a quick fix here would be to:

Am I right? So the rounding was there for a reason, I don't fully get why. If we are checking if the center frequency falls in a specific band interval, why do we require rounding? To prevent the creation of a lot of bands in the case the bandwidth is very small?

timstaley commented 8 years ago

PDF dump of the original issue, for posterity: Issue #4801 Cycle0 TraP RSM images incorrectly separated into bands - Trap - LOFAR Issue Tracker.pdf - contains quite useful explanations of the reasoning.

bartscheers commented 8 years ago

I think it is important to keep in mind that fluxes from the same frequency band can be compared with each other. We average fluxes that fall in the same band. When that is not allowed anymore (who decides that has to define the bands...), they should fall in separate bands.

gijzelaerr commented 8 years ago

So maybe I'm missing something, but I don't fully understand why we need to round the centre frequency if we later check if it falls within a bandwidth interval. Before there was a problem due to small fluctuations in the centre frequency, but it looks like we applied 2 solutions to that? round it and check if it fall within an interval.

gijzelaerr commented 8 years ago

so since this issue is important for @AntoniaR and @ycendes can you have a look at my comments and think about what is the right thing to do here?

ycendes commented 8 years ago

I wasn't getting notifications for this issue for some reason until you directly mentioned me, sorry. :( But yeah, I'm happy just doing with what you said and not rounding the central frequency- but then, I'm not a TraP expert and don't know the details behind why it was decided technically to do this.

timstaley commented 8 years ago

@ycendes Original reasoning was mostly 'this seems to work' IIRC - see PDF linked in prior comment.

AntoniaR commented 8 years ago

@gijzelaerr I've had a read through and think I agree with your solution but with a slight modification to the method outlined. i.e.:

  1. Stop rounding to the nearest MHz
  2. Take the bandwidth from the image metadata and assign two observations to the same frequency band if their bandwidths overlap
  3. Enable a user defined maximum bandwidth in the parameters file to enable the prevention of the issue that John mentioned on 18th Dec. Where "0.0" means use the image metadata with no maximum value and otherwise use the float given to define the maximum bandwidth. Does that sound reasonable to everyone?
gijzelaerr commented 8 years ago

The SQL functions have become such a nightmare, I see no other way than to rewrite the whole InsertImage, getBand and maybe when we are busy all other functions into pure Python.

This will cause a bit of a delay in solving this issue, hope to finish this somewhere next week.

gijzelaerr commented 8 years ago

this has been implemented and is in the curren master branch, it will be part of the upcoming TraP 3.2. Please reopen this issue if it doesn't work properly.