Skip to content

Conversation

@davidhassell
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 16, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


David Hassell seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jswhit
Copy link
Collaborator

jswhit commented Jul 16, 2018

Looks good, thanks @davidhassell.

Just needs a Changelog entry.

@akrherz
Copy link

akrherz commented Jul 16, 2018

Just a comment from the peanut gallery here and I know API design is hard, but the name set_auto_array_type does not seem intuitive to me. This setting is controlling whether variables should always be numpy masked arrays or not. Currently set_auto_mask(self, True_or_False) exists, it seems like a companion to this would be set_always_mask(self, True_or_False) ?

Just my unqualified two cents here.

@davidhassell
Copy link
Author

Sounds good. set_always_mask is better than what I had, I think. I'll give it a while and then update the pull request if there are no objections.

@davidhassell
Copy link
Author

@jswhit, are you happy to rename the methods, as per @akrherz's suggestion?

@jswhit
Copy link
Collaborator

jswhit commented Jul 16, 2018

Not quite clear on what set_always_mask would do. Would it be True but default, and to enable the old behavior it would be set to False?

@jswhit
Copy link
Collaborator

jswhit commented Jul 16, 2018

as per the discussion in issue #808, please go ahead and change

            # issue #785: always return masked array, if no values masked
            # set mask=False.
            data = ma.masked_array(data,mask=False,fill_value=fill_value)

to

            # issue #785: always return masked array by default, if no values masked
            data = ma.masked_array(data)

@davidhassell
Copy link
Author

Yes - The logic needs reversing when we rename to set_auto_array_type to set_always_mask. The default would be True, signifying that masked arrays are returned in every instance. I'll add this.

@jswhit
Copy link
Collaborator

jswhit commented Jul 17, 2018

Looks like tst_multifile.py is failing somewhere in cftime. Restoring the 'mask=False' fixes it.

@davidhassell
Copy link
Author

I suggest, then, that we restore mask=False here and revisit it in #808 (i.e. see if we can fix the downstream issues in cftime as pull request on that ticket (which I'm happy to look at, if you like)). I'll put mask=False back in this ticket's PR. Thanks

@jswhit jswhit merged commit a87e2df into Unidata:master Jul 20, 2018
jameshiebert added a commit to pacificclimate/climate-explorer-backend that referenced this pull request Jul 2, 2020
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.

4 participants