jacobseiler
commented
4 years ago Hey @1313e thanks for your thoughts and insights.
I think there's 2 things that should be discussed here.
- Resolving how to do what you want (your "situation" dot points),
- Comments/thoughts on the package structure
For number 2, I am still processing your comments and thinking which aspects I agree/disagree on. I will make a more in-depth reply or code modifications in a future update. It should be noted that majority of the package was written in 2019 during my thesis write-up, with major rewrites occurring in December last year and again this year in April-ish, with me having pretty much not using the package at all in the meantime. As a result, its not surprising that there is a lot of disconnect and conflicting ideologies as my coding style/priorities changed.
For number 1, I have recently just merged into Master a very big update to the API format. In particular, there is now a more simplified access flow. Your scenario, where you want to compute the stellar mass function, is covered in the documentation. Please let me know if it works/does not work for what you want.
manodeep
1313e
darrencroton
In order to get SAGE analyzed using PRISM, I have started writing a
SAGELinkwrapper class that PRISM can use for that. As I therefore require to be able to process the output from SAGE, in this case calculating the SMF, I would like to usesage-analysisto do exactly that. However, I find myself getting incredibly frustrated with using it, mostly due to the different classes in the package being very disconnected from each other. I would therefore like to share my thoughts on how this can be improved (note that they apply to both themasterand thedevbranch).Before I do so, I would like to share what exactly the situation is:
According to the documentation, I require two things: An instance of the
Modelclass (which stores all model information); and an instance of a subclass of theDataClassclass (which handles the model data), in my case theSageHdf5Dataclass. So, I first attempt to initialize theModelclass. Surprisingly enough, on themasterbranch, theModelclass takes no mandatory arguments, even though it cannot be used if it is not connected to output of SAGE. After all, it cannot run SAGE itself, so it must be connected to output. On thedevbranch on the other hand, theModelclass takes many arguments that are not required in any way, as they are saved in the SAGE output already. Therefore, theModelclass should only take 1 argument: The path to the master HDF5-file with the SAGE output. Everything else is already known at that point once that file is read.This brings me to the second thing I noticed: No data is read. If I attempt to initialize the
SageHdf5Dataclass, using an instance ofModel(this should actually be done automatically upon me providing the path to the output file), nothing actually happens. TheSageHdf5Datadoes not read in any data or attributes stored in the HDF5-file at all. This is strange, as theModelclass requires these attributes. The only thing that happens is that the open HDF5-file is stored inModel.hdf5_file, but nothing is read from it.Additionally, it is strange that the
Modelinstance that I provide toSageHdf5Datadoes not get stored as an attribute in the instance (and vice-versa). This means that any time that the latter instance required the former, I have to provide it. Even though it knows what instance should be used, as it was provided during initialization.This becomes a problem when I try to use
calc_SMF(https://github.com/sage-home/sage-analysis/blob/master/sage_analysis/example_calcs.py#L24-L60). First of all, this function should be a method ofModel. Not only because this function requires an instance of it, but also because it modifies attributes in theModelinstance. The latter is only allowed by methods of that instance. Never by a stand-alone function. In case of a stand-alone function, the results should simply be returned instead of saved in the instance.Secondly, the function requires the
galsargument, which can be obtained fromDataClass.read_gals, which again requires theModelinstance. Why does thecalc_SMFfunction not simply ask for the snapshot the SMF should be calculated for? IfModelcontains a pointer to the instance ofDataClass(and vice-versa), then all required information is there.However, even if I provide all this information,
calc_SMFstill cannot be executed. After all, theModelinstance does not have thehubble_hproperty set, as no attributes of the SAGE model were read in automatically anywhere. This is also weird, as instance properties should ALWAYS be set. Either to a default value or to a value that was determined somewhere. Getting an instance property should never raise an error, in any situation.Unless I have made major errors in my process described above, this is not acceptable. A user would require too much knowledge and effort to perform simple tasks, that the programmer can do for them already. In case of
sage-analysis, the biggest problem is that theModelandDataClassclasses are not connected to each other in any way, even though they require each other in order to function. Besides that, a user would expect that when a class is initialized that serves to handle the data stored in a specific file, that it then also reads that data whenever necessary. It should not be necessary to read that data separately, and also assign it separately, as that completely negates the purpose of said class.What I recommend doing is going over all the definitions in the entire package, and check everywhere whether it makes sense that user input is required for something. A user should ideally never have to provide any information more than once, directly or indirectly. Making sure that it works that way will significantly improve the user experience.
Let me know if there are any questions that I need to answer, or if more information is required.