search

TeamCOMPAS / COMPAS

COMPAS rapid binary population synthesis code
http://compas.science
MIT License
64 stars 64 forks source link

Documentation refers to outdated directory structure #1009

Open ilyamandel opened 8 months ago

ilyamandel commented 8 months ago

The COMPAS documentation refers to an outdated directory structure in at least one place: https://compas.readthedocs.io/en/latest/pages/User%20guide/Post-processing/post-processing.html mentions the postProcessing directory, which no longer exists. We should check for other such instances and update appropriately.

avivajpeyi commented 8 months ago

Looks like this exists in 3 places:

Screen Shot 2023-10-30 at 1 26 55 pm
avivajpeyi commented 8 months ago

Hmm the "Workgin with HDF5 in python" references a python utility h5rewrite that isnt documented/tested elsewhere. Same goes for printCompasDetails, getEventHistory, getEventStrings, in "Basic Compas Data Analysis".

Should untested/unmaintained scripts really be present in the documentation? Are these used/planned on being maintained? Otherwise, i think it may be good to just remove those references from the nteboks, make the notebooks shorter and easier to maintain.

jeffriley commented 8 months ago

I think h5rewrite was something @cneijssel wrote a while ago. I've never used it, so I'm not completely sure of what it was for - I think it allowed users to copy hdf5 files and exclude specified datasets - but I'm not sure how up-to-date it is with the current COMPAS hdf5 file format/content. (EDIT: if that was the functionality, and it would be useful to have, h5copy could be modified to do that relatively easily (it already has an exclude_group arg that excludes groups during the copy - extending that to excluding datasets within a group wouldn't be too difficult).

As long as you're in that area of the documentation (:-)), we could draw users' attention to h5repack - it's part of hdf5-tools and it will repack an hdf5 file to remove wasted space (a bit like the disk defrag operation). To install it on Ubuntu use apt-get install hdf5-tools.

I don't think I've ever heard of printCompasDetails, getEventHistory, or getEventStrings (I hope I didn't write any of them... if I did I have purged them from my memory...).

No, we probably shouldn't have any tools that we haven't committed to maintaining in the documentation. We have, or we used to have, a directory for tools that we provide but don't guarantee we'll maintain.

FloorBroekgaarden commented 8 months ago

getEventHistory, getEventStrings were functions written by Coen to understand the formation history. I could rewrite them to work with current version of COMPAS data if we think there is a need for this? Otherwise (or in any case) putting it under some non-maintained code might be best...

avivajpeyi commented 8 months ago

putting it under some non-maintained code might be best...

The code exists in the folder: misc/unsupported_utils

If this is unsupported, (assuming we don't have to re-write this stuff) would it be better just to remove it from the main COMPAS repo and place it in an 'unsuppported_tools' repo or smth?

jeffriley commented 8 months ago

If this is unsupported, (assuming we don't have to re-write this stuff) would it be better just to remove it from the main COMPAS repo and place it in an 'unsuppported_tools' repo or smth?

I don't think we want users to have to go hunting for tools - users get these tools when they clone the repo don't they? I think that's what we want. However, I'm a long way from being a github expert (or even github knowledgeable really), so if there's a good way of putting unsupported tools somewhere else but allowing users seamless/easy access to them, go for it. I think now they don't even have to know they want them - they just get them :-)

reinhold-willcox commented 8 months ago

printCompasDetails, getEventHistory, getEventStrings were written by me as data inspection tools. I think they are super useful, but they were put in the misc/unsupported_utils dir when we reorganzied last year, since they weren't maintained at the level of the other scripts. I am happy to have them removed from the Data Analysis notebooks if they are breaking the notebook functionality, but I don't think they should be removed from the repo completely. @avivajpeyi You made these points when we restructured, which is why we decided to leave them in the unsupported_utils folder. I can do a demo on them on a call at some point if it would be helpful.

ilyamandel commented 2 months ago

I was going to fix this when going through other documentation issues in #1108 , but it seems there's a lot more discussion here that I don't quite have time to get into right now. The basic issue I highlighted at the start still exists, though. Can one of the participants in this discussion (@avivajpeyi or @jeffriley ) please follow up on this issue?

avivajpeyi commented 2 months ago

Ah, ok can do @ilyamandel ! @jeffriley -- can we get on a call this week to figure out how to proceed?