-
Notifications
You must be signed in to change notification settings - Fork 1
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
Documentation read through #216
Comments
hello Rafael, I am looking in detail to Intermediate_usage_notebook.ipynb. Here are my notes to improve few things: In the cell "Update the config", I would remove the following lines. Not really useful in this context. I think that the one with the Z_STEP is sufficient to understand how to change a keyword. In "Download the required SEDs", I don't understand why do we need to specify the "additional_files". Is it not going to look for everything which is missing in the config? I don't understand what do you mean by "The additional extinction laws for galaxies are not in the principle config". So, how do we know the ones we need to specify and the ones which are downloaded automatically? curl -s -o COSMOS.para https://github.com/lephare-photoz/lephare-data/blob/main/examples/COSMOS.para When using "config = lp.read_config("./COSMOS.para")" in "Update the config", the next "lp.data_retrieval.get_auxiliary_data(" fails. "Getting new filters" Update filter list "Run prepare" "Run process" "Run process" "Lower level functionality" |
Here are my notes to improve few things in Example_full_run.ipynb: "Example full run" Remove the old text: "Set up the parameters" I would put a smaller step of 0.04 to have results which are nice, without takingf too much time. "Create filter library" I think that these two lines should be put before, at the end of "Set up the parameters" "Run the photoz" It is written that we are limited to the 100 first sources. But 10 is indicated in the following cells. I would put something large, like 1000. t[:5] is duplicated. A plot photo-z versus spec-z would be nice. |
Here are my note for the command lines section of the doc. "Advanced Usage via Command Line Interface" immeidately -> immediately "Legacy code overview and main features" This paragraph should be at the beginning of the page, at the beginning of "Advanced Usage via Command Line Interface". Remove "compute extinction correction" from the sedtolib item. That's done in mag_gal. sedtolib convert all templates in a common format and unit. . doesn't appear as a link. "Build only the C++ executables with make (historical method)" I would merge this two sections and create a single one with this title "Build only the C++ executables (historical method)". This should be the last paragraph of this page. We can present it as the historical method but also the method which doesn't require any python dependency in case of difficulty in the code installation (if working only with command lines). The link is for gitlab, not github. TBC. Also, it is required to have cloned the LEPHARE-data to make it works. Also, I think that the two keywords need to be define by hand to make it work Build the C++ executables and the python module with cmake and setuptools I think that this paragraph should be removed. That's a duplication of "Developer Guide" in "Getting Started". And the one in developper guide is more accurate (some typos in this one, and the "conda install cxx-compilers" missing). So, I would simply removed this paragraph. Quick Start It is a duplication of what do you have at the beginning of the page. I would remove this paragraph, just making sure that there is not something missing in "Advanced Usage via Command Line Interface" |
Here are some comments for the page "getting started". The page is really clear. I would move everything which is in "Advanced Usage" within "Example Usage", just after the note. for me, that's necessary to have such understanding to run the code (not advanced usage). In "Auxiliary Data and the Environment Variables", it would be nice to include the git clone line to be used. |
Here are some changes for the keywords. I review all keywords and try to take the best explanation between this version and the detailed documentation. sedtolib GAL_SED/QSO_SED/STAR_SED GAL_FSCALE/QSO_SCALE/STAR_SCALE SEL_AGE AGE_RANGE mag_gal COSMOLOGY EXTINC_LAW GAL_LIB_IN / QSO_LIB_IN / STAR_LIB_IN GAL_LIB_OUT / QSO_LIB_OUT / STAR_LIB_OUT LIB_ASCII ADD_DUSTEM zphota It would be nice to split this table in smaller pieces, according to what is done in the detailed description: CAT_IN ERR_FACTOR BD_SCALE/ GLB_CONTEXT / FORB_CONTEXT MASS_SCALE MAG_ABS MAG_ABS_QSO NZ_PRIOR ZFIX EXTERNALZ_FILE DZ_WIN SPEC_OUT CHI2_OUT PDZ_TYPE MABS_METHOD Z_METHOD MABS_REF ADDITIONAL_MAG M_REF and RF_COLORS: never tested seriously. Not sure to make them appearing. M_REF is really a bad name since redondant with above. APPLY_SYSSHIFT ADAPT_BAND RM _DISCREPENT_BD Z_RANGE EBV_RANGE I think I would remove the full section: |
I have implemented most of these. We can talk today about a couple of points. The tables of keywords should probably be alphabetized. By wrapping the descriptions they are more readable now. I can't see an obvious way to break the zphota table. Is there a logical way to break it in two? |
We are conducting a read through of the documentation and want to make a number of changes as we go. This issue will lead to a branch that will be used for this Documentation work.
The text was updated successfully, but these errors were encountered: