Improves FITS storage: Adds NIHAO support #132
Conversation
rubix/core/fits.py
Outdated
| # Define output filename | ||
| if config["simulation"]["name"].lower() == "illustris": | ||
| output_filename = f"{filepath}{config['simulation']['name']}_id{config['data']['load_galaxy_args']['id']}_snap{config['data']['args']['snapshot']}_{parttype}_subset{config['data']['subset']['use_subset']}.fits" | ||
| else: # NIHAO | ||
| output_filename = f"{filepath}{config['simulation']['name']}_snap{config['data']['args'].get('snapshot', 'unknown')}_{parttype}_subset{config['data'].get('subset', {}).get('use_subset', 'unknown')}.fits" | ||
|
|
There was a problem hiding this comment.
why this different syntax here? Can't we homogenise it?
why does NIHAO need the .get method?
I would argue for we homogenise this to use the same keywords and loading procedure for both galaxies. If needed we just create an ID field for NIHAO as well.
There was a problem hiding this comment.
For Illustris we assume that all keys are present, so we can access them directly. For NIHAO, some keys are missing (snapshot and use_subset), therefore I used .get to provide default values and avoid errors.
There was a problem hiding this comment.
use_subset is also present as key for NIHAO
There was a problem hiding this comment.
snapshot: we are currently using 1024 (indicated in the file path name), we can store this as parameter
There was a problem hiding this comment.
The thing is that we have not directly the galaxy ID for NIHAO, but we could use the name g2.8e11 or so as galaxy ID in case of NIHAO. There are two ways, we could specify it in the config as key as galaxy id or we extract it from the file name (same for snapshot)
There was a problem hiding this comment.
sorry, I meant snapshot and galaxy ID are missing, so it led to a keyerror
I'll take a look at how to make it homogeneous.
…e galaxy ID and snapshot from the simulation path)
anschaible
left a comment
anschaible
left a comment
There was a problem hiding this comment.
For me it is fine to have an helper function to extract the galaxy id and snapshot from the file path. In my opinion, we will not get rid of adjusting the header for each simulation type a bit.
rubix/core/fits.py
Outdated
| from matplotlib.colors import LogNorm | ||
| import os | ||
|
|
||
| def extract_info_from_path(sim_path): |
There was a problem hiding this comment.
looks, the only problem I see with this is that it implicitly assumes that path has a part that is snap_.
what do we do if that is not the case?
I have no good idea how to deal with this as it would always be up to the user to specify this, but maybe a better solution is to simply require the user to set in the config the fields
config["data"]["load_galaxy_args"]["id"] config["data"]["args"]["snapshot"]
what do you think?
This would make the fits header independent of the users data paths...
There was a problem hiding this comment.
Yeah that makes sense.
I have updated the implementation, so now the galaxy id and snapshot are taken directly from the config which is now updated. Also in the fits_file.ipynb we can now select if we want to use NIHAO or Illustris. I thought about creating a new fits_file.ipynb just for NIHAO, but since only the config changes, I implemented both in the same ipynb, @TobiBu @anschaible what do you think about that?
No description provided.