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

use utilix for client #356

Merged
merged 5 commits into from Nov 24, 2020
Merged

use utilix for client #356

merged 5 commits into from Nov 24, 2020

Conversation

JoranAngevaare
Copy link
Member

@JoranAngevaare JoranAngevaare commented Nov 23, 2020

What is the problem / what does the code in this PR do
Change the way we init the pymongo client for the corrections. NB: the init is now done on the straxen side (makes more sense to do it in the nT specific package).

Can you briefly describe how it works?
Given recent changes (XENONnT/straxen#163), it makes more sense to use a centralized initiation for the pymongo client (i.e. utilix).

Can you give a minimal working example (or illustrate with a figure)?
All should work just as before. It's only when someone uses this script outside of the straxen environment that one should initialize a client (see XENONnT/straxen#288).

@JoranAngevaare
Copy link
Member Author

Also trying to tag Evan here but cannot do because he is not part of the Axfoundation

@ahiguera-mx
Copy link
Contributor

I'm fine with this change. Just want to express some small comments. To me it is more transparent to the user specify every object individually and since utilix is a straxen specific not sure this goes along with strax's philosophy of being experiment agnostic

@JoranAngevaare
Copy link
Member Author

So if you just know about strax, the client can be anyone's, a client you get from admix, a client you construct yourself, whatever so it's not experiment specific at all.

If you think this too abstract, I can also propose this:

    def __init__(self, client=None, host=None, username=None, password=None, database_name='corrections'):
        """
        :param client: DB pymongo client
        <update>
        :param database_name: DB name
        """
        if (client is not None) and (host is None and username is None and password is None):
             self.client = client
        elif (client is None) and (host is not None and username is not None and password is not None):
             self.client = pymongo.MongoClient(host=host, username=username, password=password)
        else:
              raise ValueError('Can only init using *either* the client or the combination of'
                               'host+username+password, not both')
              
        self.database_name = database_name

if this makes more sense to you.

@WenzDaniel
Copy link
Collaborator

Since you have it already available, maybe the second option is a bit more flexible while not much more complex (I would also have been fine with the first approach). May I just ask you to be a bit more explicit with the description of the "client" argument. Because we are very explicitly expecting here an instance ala pymongo.MongoClient. I am not sure if there are any other pymongo libraries which could lead to misunderstandings, but more documentation never hurts.

@ershockley
Copy link
Contributor

Thanks for the PR and the invite to the organization @jorana. I personally like the simplicity of the original PR, though can see the merits of having an either/or implementation. I agree with Daniel on the documentation.

@JoranAngevaare
Copy link
Member Author

Thanks @ahiguera-mx @ershockley @WenzDaniel for the review. I changed the PR accordingly. Please add a positive review if you think the concerns have been appropriately addressed.

Copy link
Collaborator

@WenzDaniel WenzDaniel left a comment

Choose a reason for hiding this comment

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

Looks nice, thanks a lot.

@ahiguera-mx
Copy link
Contributor

thanks, I'm fine with the changes

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

4 participants