-
Notifications
You must be signed in to change notification settings - Fork 5
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
Integrate the primitive groups of degree 4096 to 8191 into PrimGrp #52
Conversation
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.
Looks Good. Thank you! Alexander
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.
Thank you for your contribution. However, there are some minor issues with this PR in its current form. Perhaps you would be willing to look into that, otherwise we'll have to sort it out later.
lib/primitiv.gi
Outdated
strm:=InputTextFile(filename);; | ||
if strm = fail then |
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.
With the above changes, you can just test filename
, and avoid using streams:
strm:=InputTextFile(filename);; | |
if strm = fail then | |
if filename = fail then |
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 had to put strm back so that I could read the contents of the file with ReadAll since it throws an error about a missing ; if I try to use Read. I changed the if statement to check if the filename has failed as you suggested, however.
The archive on Zenodo is unnecessarily large. It contains many .gz files which are not actually compressed. At the very least, all There is also a 5MB Just fixing the above reduces the data size to 1.7GB (I used options It is a bit annoying to clean this up with a script because there is a single directory containing ~29777 files. Various filesystems have trouble with such large directories. A natural way to avoid this would be to add subdirectories for each degree. |
Turns out 20040 out of the whole 29776 are not actually compressed. After uncompressing them all, the result actually only takes 3.0G on my disk according to Recompressing with I'll try recompressing all files using |
With zopfli it goes down to ~1.0G |
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Hi Max, thanks for the suggested changes. I have accepted most of them already. There is one more that I need to address but it will require me to make some modifications. I'll do so shortly. Also, the files are supposed to be compressed... So I will make sure they are compressed and update the Zenodo repository once I have done so. |
I have made sure the files are properly compressed (using zopfli) and udated the Zenodo arxiv. It is now just over 1GB. I have addressed each of the other suggested edits. Is everything ok now? |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #52 +/- ##
==========================================
- Coverage 99.92% 99.91% -0.01%
==========================================
Files 46 46
Lines 382928 383085 +157
==========================================
+ Hits 382623 382764 +141
- Misses 305 321 +16
|
Thanks @jesselansdown I was busy elsewhere and forgot about this PR. It looks fine now! On the long run I'd like to transition those data files to a new file format that maybe does not depend on parsing GAP code (i.e. so that other system could import it). Perhaps it would even be possibly to find a common format for old and new data. There are more things (making the data available with a finer granularity so that only parts which are needed can be fetched online on demand; perhaps even integrating this into a "real" DB, a website, etc.) All of that is not meant to block this PR, just saying what I have in mind for future work. |
No worries, glad everything is ok now! My main concern was to make the data available and compatible with the current library so that people can begin to use it already, but I agree that the data format could be improved in the future. |
The primitive groups have been made available on Zenodo and additional properties computed to make them compatible with PrimGrp. The ability to load these groups have been added to PrimGrp and the other functions modified as needed to accommodate the new groups.