Skip to content

Change candidate file writing to facilitate faster single candidate retrieval#115

Merged
larskuenkel merged 1 commit into
mainfrom
redo_spcc_writing
Mar 12, 2025
Merged

Change candidate file writing to facilitate faster single candidate retrieval#115
larskuenkel merged 1 commit into
mainfrom
redo_spcc_writing

Conversation

@larskuenkel

@larskuenkel larskuenkel commented Mar 10, 2025

Copy link
Copy Markdown
Contributor

This PR includes the following minor changes:

  • Changed spc.as_dict to just use spc.__ dict__ since this seems to work currently. I think the enumerate entries were a problem at some point, but in my current environment it worked without that
  • Replaced hhat mentions with ffa in SearchAlgorithm
  • Removed block used for backwards compatibility in spcc reading. That change was 3 years ago and I doubt anyone uses candidates this old with the current code.
  • Fixed reading candidates twice in spcc.read. Apprently if you run data["candidate_dicts"] twice, when data is a npz file loaded with np.load, it will load the candidates twice.

The major change is the introduction of the split_cands argument in spcc.write. When this is used, the different candidates are saved separately into different keys of the npz file. Since np.load loads npz files lazily, this allows quick retrieval of individual candidates without loading all candidates. But when loading all candidates it is a bit slower. From my tests up to 50% but it is still faster than previously because of the double reading issue.

Whether this option is really used, depends on how often each files is read in which mode.

From my parts of the pipeline I read all candidates during the multi pointing clustering, but I do not write the individual candidates into the mp candidates files, to not write redundant data. This would be slower with this option.
But as part of my ML efforts when I create test sets, I read sp candidates based on the mp candidates. In this case the new method is probably more than 10 times faster usually.
Are there any more reads of the sp candidates?

A picture illustrating the performance of full and partial reading of SinglePointingCandidateCollections based on the method.

image

Any thoughts?

@kmjc

kmjc commented Mar 11, 2025

Copy link
Copy Markdown

Would you mind double checking the detection statistic still reads and writes correctly with the new dict method? As I recall that was the pain in the patella when reading/writing candidates.

The observation id used to do the same thing but doesn't anymore.

@kmjc

kmjc commented Mar 11, 2025

Copy link
Copy Markdown

It sounds like this is a pretty good trade off, but I'm not 100% sure.

Is this only a thing you're doing to make test ML sets? Or do you think it will it be a regular part of the ML pipeline?
And how long-ish would it take to read in a whole day's worth of candidates in the multipoint clustering? If it's super long already then it might not be worth slowing it down.

@larskuenkel

larskuenkel commented Mar 11, 2025

Copy link
Copy Markdown
Contributor Author

Would you mind double checking the detection statistic still reads and writes correctly with the new dict method? As I recall that was the pain in the patella when reading/writing candidates.

The observation id used to do the same thing but doesn't anymore.

I think it should be fine.
image

I remember there having been some trouble with it as well, but it now worked.

I wanted to also check if I can improve things for the multi-pointing writing, but not sure yet what exactly to do there. A while ago I changed it to use attr.asdict() instead of the as_dict method to but just .dict is faster.
But still testing things around. I noticed some mp candidates were 400 MB large lol, even without saving all the candidates arrays.

I think in general one can just just use the .dict method and if something does not work, fix it in the post init.

pain in the patella

I have not heard that expression, but I have luxated my patella once which was the absolute worst.^^

@larskuenkel

larskuenkel commented Mar 11, 2025

Copy link
Copy Markdown
Contributor Author

It sounds like this is a pretty good trade off, but I'm not 100% sure.

Is this only a thing you're doing to make test ML sets? Or do you think it will it be a regular part of the ML pipeline? And how long-ish would it take to read in a whole day's worth of candidates in the multipoint clustering? If it's super long already then it might not be worth slowing it down.

Hmm, I checked the stack search for 10k pointings and there reading took 6 minutes and clustering 8. With this PR with the new format I suppose reading would take 3 minutes( with the old format) and 4 with the new one.

Checked a single day with 2500 pointings and there reading and clustering took like 70 and 150 seconds respectively.

So, I think reading there is not the most expensive operation. If we want to speedup that operation I could also consider splitting up candidates further up in the npz file, so we don't need to read all the arrays if we do not need them. I'll think about it and test to see if that makes sense.

I am still a bit torn whether I want to throw the classifiers on all single pointing candidates or just a subset of those based on the multi pointing result. In the latter case the performance would profit greatly from using the new format.

@kmjc kmjc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup seems totally fine to me then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants