Skip to content

Conversation

@jmbenlloch
Copy link
Contributor

Whenever a new calibration is done some tables are changed in our MySQL server, that are later reflected in the SQLite file uploaded to this repository (examples in #765, #759, #754, etc.). Usually I'm the one responsible of reviewing those PRs. Besides checking all tests passed in Travis there is an extra step to ensure everything is ok and there was no human error in the process of creating the sqlite copy. This PR shows a way to do it automatically.

The calibration in the database affects the tables ChannelGain, ChannelMask and SipmNoisePDF. All of them are indexed using a MinRun and a MaxRun, delimiting the range of runs to which those values are applicable. The set of (MinRun, MaxRun) for each of those tables must be the same in our MySQL server and in the sqlite3 file included in the corresponding PR.

So far, I have been doing that step manually. This PR includes a Github action to do that check automatically. It will be triggered only for PR's modifying some *.sqlite3 file. This is what it does:

  • Detect which is the updated database by parsing the name of the updated file. So it is useful for NEW, DEMOPP, etc.
  • Retrieve the (MinRun, MaxRun) set for the relevant tables and checks the SQLite file contains the same thing both for SiPMs and PMTs (separately).
  • It does NOT check all the values (too much data), but only that there is consistency between the latest calibration run number in both databases.

As illustration I have already included the code in this PR into my fork's master and created several PR's two show you how it works:

I think this automation will simplify the reviewing of calibration PR's (which are common) and can also be useful as a proof of concept for tests/tasks that we want to run occasionally and do not fit well in the pytest scheme. This can be the first Github action included in IC, but there may be more in the future.

PS: In the sample images you can see a check called Python package / build (3.7) (pull_request), I have updated that name to Calibration check / build (3.7) (pull_request)

@carmenromo
Copy link
Collaborator

Amazing @jmbenlloch!!! Thank you!!!

Retrieve the (MinRun, MaxRun) set for the relevant tables and checks the SQLite file contains the same thing both for SiPMs and PMTs (separately).

In the case of DEMO we do not calibrate always both type of sensors. Here you mean that they are independent, right? Or does it check that the run numbers match for SiPMs and PMTs?

@jmbenlloch
Copy link
Contributor Author

In the case of DEMO we do not calibrate always both type of sensors. Here you mean that they are independent, right? Or does it check that the run numbers match for SiPMs and PMTs?

The code will extract from the database the (MinRun, MaxRun) set first for the SiPMs (SensorID > 100) and then for the PMTs (SensorID < 100). In this way there shouldn't be any problem if you have updated both SiPMs and PMTs or only one of them.

@jacg
Copy link
Collaborator

jacg commented Jan 15, 2021

I doubt I'll have much more to say on this, but I think that someone who is familiar with the task that is being automated, should have a look at exactly what is being verified, whether it makes sense, and whether anything has been overlooked.

AFAIAC: LGTM.

@jmbenlloch
Copy link
Contributor Author

I doubt I'll have much more to say on this, but I think that someone who is familiar with the task that is being automated, should have a look at exactly what is being verified, whether it makes sense, and whether anything has been overlooked.

Thanks @jacg. I have squashed all the commits. Maybe @carmenromo can have a look a give the final review.

@jmbenlloch jmbenlloch requested a review from carmenromo January 15, 2021 11:17
Copy link
Collaborator

@carmenromo carmenromo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested to @jmbenlloch to make another test to check that there are no gaps between the run numbers. However, given the large number of required exceptions we agreed on leaving it for the future, after reviewing the database itself.

I have reviewed the function that checks the gain update and it does its work, so I approve!

@carmenromo carmenromo merged commit 91a5322 into next-exp:master Jan 18, 2021
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.

3 participants