-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add necessary code to run linear QuasarNET #2230
Conversation
py/desispec/scripts/qsoqn.py
Outdated
else: # logarithmic grid | ||
linear_weights = False | ||
wave_to_use = wave | ||
|
||
model_QN = load_model(model_QN_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit nervous about using the filename itself to make the linear vs. log determination. Could that be derived from the contents of the file itself and an interface like:
model_QN, wave_to_use = load_model(model_QN_path)
...
data, index_with_QN = load_desi_coadd(spectra_name, sel_to_QN, wave_to_use)
i.e. instead of passing around boolean flags and multiple wavelength grid options and deriving what to do from a filename, have the load_model
tell you exactly which wavelength grid to use, and pass that into load_desi_coadd
to enforce using that grid. Would that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make this happen but I should warn you that it would only be slightly less fragile while shifting the fragility into QuasarNP. Since the model output saved from tensorflow has no determination whether or not the input grid is logarithmic vs linear, the only way that QuasarNP would determine it within load_model would be to check whether or not the expected input dimensionality of the dataset is 443 (for logarithmic) or 458 (for linear). I'm mildly wary of this because it would be a significant api change that is backwards breaking (i.e. any old code that uses load_model will break with the additional return value) but if you think it's the best option I can implement it somewhat easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documenting a few items from a Slack conversation:
- let's post-process the new model file to include a new data element with the wavelengths used. Code can then use that to know exactly what wavelengths to use without guessing. If you don't want to touch the old model file, ok, the code could assume that if the "wavelengths" element is missing and the weights have size 443 then it is an old model file.
- desispec.zmtl is another place where QuasarNP is load and run, which needs to be updated
- Since Dylan + the DESI pipeline are the only known users of QuasarNP, I'm fine with breaking backwards compatibility of load_model to also return the wavelength array (since adapting to the change is also a simple end-user fix). If we want to be really conservative, we could also add a new function
load_model_and_wavelengths
or a new optionload_model(filename, return_wavelengths=False)
that is backwards compatible but can opt-in to getting the wavelengths too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated QuasarNP is currently in a branch at https://github.com/desihub/QuasarNP/tree/auto_derive_linear, but can/will be pulled into the main branch and tagged. With this change:
load_model
now returns the wavelength grid used for the given weights file, auto assuming logarithmic grid unless specified within the weights file. qsoqn uses this grid for running QuasarNP. I've elected to just break backwards compat, we incremented the version number to 0.2.x anyway, if anyone needs backwards compatible with regards to return types and values API they can use 0.1.6. I'll make note of this in the next QuasarNP release notes.- There is a script in QuasarNP: quasarnp/scripts/post_process_weights.py which can add wavelength information (linear or log) to weights files for loading with quasarnp
I've also made what I believe to be the requisite changes to zmtl.py, although I haven't had a chance yet to test and verify that it works as intended...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with the pipeline + desihub/QuasarNP#7 branch. Looks good, merging.
This PR makes the necessary changes to run the linear version of QuasarNET rather than the logarithmic. The code will automatically determine whether to use the linear grid or not by checking if the word "linear" appears in the weights file name.
This means that script is backwards compatible, such that if the environment variable $QN_MODEL_FILE is set to the old weights file (at
science/lya/qn_models/boss_dr12/qn_train_coadd_indtrain_0_0_boss10.h5
) then this script should reproduce previous results. Also unchanged is the default weights file, if no QN_MODEL_FILE is provided qsoqn.py will still load the old weights file by default. I am not opposed to changing this.The new linear QuasarNET weights file is now located at
science/lya/qn_models/desi/qn_train_desi_guadalupe_linear.h5
. In order to use the new weights file, $QN_MODEL_FILE should be set to the new location.I've never run QuasarNET through the afterburner before so I'm unsure exactly the best way to run and test this. In my tests on QuasarNET alone, only ~1.3% of exposures changed classification when using the new weights file versus the old one. It is likely sufficient to pick a few quasar heavy files, run qsoqn.py with both the old and new weights file, and check if there are any significant differences in classification and redshift.
Note that this code requires QuasarNP 0.1.6.