search

pynbody / genetIC

The GM initial conditions generator
GNU General Public License v3.0
21 stars 8 forks source link

Fix bug when merging IDs from two different input mappers #78

Closed apontzen closed 3 years ago

apontzen commented 3 years ago

The existing code stored particle IDs, rather than cell flags, when using merge_id_file and therefore if the input mapper changed it tried to process the old IDs again with the new mapper.

This fixes the bug by merging at the cell flag level instead.

It also introduces a slightly more readable format when processing multiple input mappers.

Martin-Rey commented 3 years ago

Hi Andrew,

Thanks for the fix, I think the new behaviour working at the flag level rather than ID level is much safer and neater.

Re output logs, it is a very nice addition to have the logged times, and I think the effort to highlight independent mapper calls works well.

Maybe we could make them stand out even more by isolating them further from the main process. E.g. by bracketing them with empty lines? Or have a "------------" line once the mapper_relative_to is finished? As an example on the new test, it is quite hard to visually grasp that "Centre of the region..." is part of the main run when quick browsing.

genetIC v1.1.0, compiled: Oct 23 2020, 11:45:53
              runtime:  Oct 23 2020, 11:46:58

git HEAD:d34a630
At the time of compilation, the following files had been modified:
  Makefile

 2020-10-23 11:46:58   
Limiting number of FFTW Threads to 8, because FFTW on Mac OS seems to become slow beyond this point.
To disable this behaviour, recompile with -DIGNORE_APPLE_FFTW_THREAD_LIMIT
OpenMP parts of the code will still run with 12 threads.

 2020-10-23 11:46:58   Initialized the base grid:
 2020-10-23 11:46:58     Box length            = 50 Mpc/h
 2020-10-23 11:46:58     n                     = 32
 2020-10-23 11:46:58     dx                    = 1.5625
 2020-10-23 11:46:58   Initialized a zoom region:
 2020-10-23 11:46:58     Subbox length         = 12.5 Mpc/h
 2020-10-23 11:46:58     n                     = 32
 2020-10-23 11:46:58     dx                    = 0.390625
 2020-10-23 11:46:58     Zoom factor           = 4
 2020-10-23 11:46:58     Num particles         = 1
 2020-10-23 11:46:58     Low-left corner in parent grid = (11, 11, 11)
 2020-10-23 11:46:58     Low-left corner (h**-1 Mpc)    = 17.1875, 17.1875, 17.1875
 2020-10-23 11:46:58     Total particles       = 32831
 2020-10-23 11:46:58   + Input mapper: computing geometry from paramfile_lores.txt ------------------------
 2020-10-23 11:46:58   | 
 2020-10-23 11:46:58   | WARNING: basegrid is a deprecated command and has been replaced by base_grid
 2020-10-23 11:46:58   | Initialized the base grid:
 2020-10-23 11:46:58   |   Box length            = 50 Mpc/h
 2020-10-23 11:46:58   |   n                     = 32
 2020-10-23 11:46:58   |   dx                    = 1.5625
 2020-10-23 11:46:58   Centre of region is 49.21875 49.21875 49.21875
 2020-10-23 11:46:58   + Input mapper: computing geometry from paramfile_ultra_lores.txt ------------------------
 2020-10-23 11:46:58   | 
 2020-10-23 11:46:58   | WARNING: basegrid is a deprecated command and has been replaced by base_grid
 2020-10-23 11:46:58   | Initialized the base grid:
 2020-10-23 11:46:58   |   Box length            = 50 Mpc/h
 2020-10-23 11:46:58   |   n                     = 32
 2020-10-23 11:46:58   |   dx                    = 1.5625
 2020-10-23 11:46:58   Centre of region is 48.5243055556 48.5243055556 1.30208333333
 2020-10-23 11:46:58   Drawing random numbers (base grid)
 2020-10-23 11:46:58   Drawing random numbers (zoom level 1)
 2020-10-23 11:46:58   Calculating particles from overdensity fields...
 2020-10-23 11:46:58   Combining information from different levels...
 2020-10-23 11:46:58   Writing output; number dm particles=32831, number gas particles=0
 2020-10-23 11:46:58   Finished writing initial conditions
apontzen commented 3 years ago

Thanks Martin, I agree. Does this now look better?

Martin-Rey commented 3 years ago

Yes I think the new spacing helps readability a lot!

Martin-Rey commented 3 years ago

Happy for you to merge whenever practical. I believe the build currently fails at test_21 because the reference output expects ------- on a line where this is none anymore in the print statement.