Skip to content
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

Hecke algebras #2189

Merged
merged 56 commits into from
Aug 2, 2017
Merged

Hecke algebras #2189

merged 56 commits into from
Aug 2, 2017

Conversation

sanni85
Copy link
Contributor

@sanni85 sanni85 commented Jul 19, 2017

This pull request contains the implementation of the Hecke Algebras database which was presented in Warwick last June.

Search and download functionalities work. Links to the modular forms database are work in progress.

Tests are going to be written after data upload. Knowls are going to be added later, once on beta.

The list of algebras in the favourite contains complete 2-adic and mod 2 data.
In the next days I will upload a complete set of levels and weight and l-adic data for those.

@JohnCremona
Copy link
Member

I had a quick look at the code (not scripts) in case anything jumped out at me. I will also look at the pages (and hope that others will too!). A couple of things:

  1. Don't pass the connection C as a parameter. There is no need, there is only one global instance. And also, you have a lot if instances of C.<database_name>.<collection_name> in the code. This is asking for trouble. Follow a Good Example and put a function somewhere, to be imported where needed, so that you can use (something like) db_orbits.find(...) and not C.hecke_algebras.hecke_algebras.find(...). So you don't need to refer to the connection name anywhere outside of these db_*() functions anyway.
  2. Your stats class does counting on-the-fly when created. That's what I used to do before @AndrewVSutherland showed me a Better Way and provided utilities to help: use the stats counting scripts in utilities to make new collections called xxxx.stats which hold these counts; put the script to update these somewhere in your upload script files, so it is trivially easy to update the stats collections whenever you change the data; and initialize your stats class from there. (It may even make having a stats class redundant. Sorry, I was the one who started these...).

@sanni85
Copy link
Contributor Author

sanni85 commented Jul 20, 2017

I will fix this and add commits.

I will do some clean up for the connection call. There are 3 collections in the same database and I am using all of them (also for stats), and that is why there are several calls...

@JohnCremona
Copy link
Member

OK, it's no problem to have 3 collections; the point is that the name of the collection should only appear once in the code. One day you'll rewrite the collection x to x.new and to test it you'll then only need to change one occurrence of x in the code. And if any other part of the system needs to access that database they just need to know one function to call.

@JohnCremona
Copy link
Member

... I like the cleaned up handling of the collections...

@sanni85
Copy link
Contributor Author

sanni85 commented Jul 21, 2017

I am working on the stats now and doing more clean up.

@sanni85
Copy link
Contributor Author

sanni85 commented Jul 21, 2017

Now stats should work as @JohnCremona was suggesting.
Added tests, also run pyflakes and done more clean up.

JohnCremona and others added 2 commits August 1, 2017 09:46
@JohnCremona
Copy link
Member

@sanni85 and I have been discussing this and some changes will be made before this is merged, so the PR can remain open.

@JohnCremona
Copy link
Member

I am merging this since that will make it easier for @sanni85 and others to keep working on it, and it is invisible.

@JohnCremona JohnCremona merged commit ba0013b into LMFDB:master Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants