-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add NEXT100 Activity/Efficiency database #806
Conversation
1f194c1
to
eae5030
Compare
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.
This PR adds a new database with the activity assumptions.
Please, DO NOT merge yet. I've seen after the approval that the automated tests related to the database are failing. I need to check what is happening. |
A few comments:
|
Not sure what you mean, the |
Yes, my bad, too little coffee in my system. |
I agree that we should add a version number column and we can think of moving the databse to NEXT100DB. But whatever we decide does not affect this PR. |
Well, not really. For sure it would affect the download program because now the table would be located in a different database, but most importantly you would need to modify this code again once we agree on the structure of the table, so why not do that already? |
I would say that the database will be composed by two tables: Activity and Efficiency, and that won't change. If I understand your point, what it is still to be decided:
If I am not wrong, the second point does not affect the PR either, since NEXT100DB/NEXT100DB is loaded independently of being inside NEXT100DB folder. |
Regarding that point, let me clarify those "folders" do not exist. You are getting that impression by looking at the databases using the web interface (phpMyAdmin). In the web, the databases that start with a common name are grouped for readability, but such groups are completely fictitious. You can have your tables either in |
Sorry, in fact this would affect the reader |
As for 1), it doesn't need to affect this PR necessarily, but since you added a function to load the data within IC, I think it makes sense to fix it here already. And as I was typing this you just replied saying that :)
rather than adding another db with just these two tables to be read. |
Ok, I see. Let me state my opinion for the final implementation:
Could you express your opinion @gonzaponte @jmbenlloch @msorel? |
The actual choice of using a version number vs. minrun/maxrun vs. any other variant is something that I think should be decided by those who have used these data for various studies in the past. They should have a better understanding of how to manage these tables' updates and have a more informed and relevant opinion on the best way to go. |
HI, thanks for moving this. Some comments from my side:
|
I agree on that. It makes more sense to have all the information related to a particular detector in the same database.
I also agree on this. Its better to have different tables.
This scheme would mirror the current tables we have been using in the spreadsheet. I think the advantage would be that all the information would be in the DB, but it may require more development from the software side. It's up to the developers/users to decide which scheme fits better your needs.
Completely agree on that.
This is not that simple to do. You can basically do two things:
Again, up to you which version suits better your needs. |
Thanks, @msorel. If you think 3 tables is more flexible it might be the preferred solution. Regarding the versioning of the tables, I think I don't understand it well. You first said that these 2/3 tables are updated independently, so they would have different version numbers, but then you said we need to tag them together. Technically, a single version number can be achieved in practice in a couple of different ways. First, using the version column, which is not a big deal, we are not talking about huge tables anyway. Then you can do two things:
I think the second one might be a good way to go, but I don't have that much experience with databases, so it might have some caveats. |
Option 1 looks less ugly to me, so I would go with that. The operation could then be like this, if I understand correctly:
What do you think? |
The solution you've described is the one I'd implement. Everything should work properly with the workflow you've described and would avoid data redundancies. |
HI @gonzaponte @jmbenlloch , looks like we agree on how to version-control individual tables, then! I have a slight preference to separate into three tables, yes. If @gondiaz @MiryamMV are fine with that you are right @gonzaponte , the scheme I have proposed does not necessarily ensure the same version number for all 2/3 tables we are talking about (table version = highest version when looping over table rows). But thinking more about it, ensuring this is probably not needed. If the specific acitivity, quantities and efficiency tables have different version numbers, it is probably OK |
Talking with @gonzaponte offline we propose to do have a single version number, the version number of the Activity-assumptions. An example might help to be more clear:
Then if we request for example Activity-assumptions version v2 we will get (v0, v2, v2), or (v3, v2, v2) if we request the last version v3. Since it is not necessary for the event-mixer, I would skip the specific-activities table for the moment. It gives an extra work since the volumes are not necesarily the ones in the G4Volume list. Do you all agree? |
Hi @gondiaz , sounds good to me, if we want to keep a global version number encompassing all tables. However, now we only two tables and not three, I understand, right? Another comment. I checked what you put in mysql as temporary table structure. you may consider switching to a more flexible table structure with four columns each. Table Activity, with total activities (in mBq):
Table Efficiency, with dimensionless efficiencies:
so first three columns are common. This structure is a little more flexible. If tomorrow we want to add a background isotope other than the four considered thus far, we won't need to change the structure of the tables. |
Yes, let's start with the two tables and add the thrid one that is not used directly by the mixer in the future. Agree with that, this structure is better suited to add more background or components in future versions but slighly worse in my opinion to work with pandas, but it can be managed. |
I agree with everything, but what's the purpose of keeping (v0, v2, v2) and (v3, v2, v2) at the same time? |
18bc8fa
to
5dc2ac8
Compare
The test processing is failing at the new test function I have added. I think that is is caused because the new table Activity in the database is not being updated in the test server. Could you take a look @jmbenlloch? The PR should be ready to review (again) by then. |
You'll have to commit the |
Absolutely, my bad. It should pass the failing test now. |
611987a
to
6d71e1d
Compare
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.
First round. Please run pyflakes
after you address these comments to check for unused variables.
Side note: |
Unless this has changed in nexus recently, all those white spaces should be underscores. I think we should be in time to enforce underscores and avoid white spaces in NEXT-100 geometry. luckily, precisely @gondiaz is our man for that! |
G4Volume names does not contain white spaces in nexus in fact. The reason why they are in the database is because I took the data from an unupdated spread sheet, without the correct G4Volume names in the last nexus revision. |
21ed856
to
30d55cd
Compare
Cosmetics
Update NEXT100DB Update NEXT100DB Update NEXT100DB
30d55cd
to
6813c62
Compare
Just some squashing and reword |
Rewrite activity reader to be more flexible Alignment
Rewrite database test Fix typo in test Rename df in test Co-authored-by: Gonzalo <gonzaponte@gmail.com> Add test for Efficiency table Add test for version number request Parametrize version in test Avoid code repetition in test Co-authored-by: Gonzalo <gonzaponte@gmail.com> Improve redability
6813c62
to
a8b0424
Compare
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.
This PR adds new tables to the database and implements the functionality to load them within IC. It also provides a couple of tests to demonstrate and check the functionality.
Great job.
#815 [author: gondiaz] Updates the activity and efficiency columns in NEXT100 database with the latest activity values from measurements and the efficiencies computed through nexus simulations. Efficiencies assume at least 2 MeV is deposited in the active volume. Recall that the previous values were dummy, in order to test PR #806. [reviewer: MiryamMV] This PR adds the correct values of activities (v3) and efficiencies to the database for NEXT100.
This PR adds a new database to IC in similar terms as is done for detector's databases.
The new database is called NEXT100ACTDB, and is already placed in a dummy version at the mysql server (mysql server).
The database contains two tables, Activity and Efficiency with same column names. This database will be likely updated in the near future, and its purpose is to serve as an input to the MC Event-Mixer which will be also developed in subsequent PRs.