Skip to content

Conversation

@marqh
Copy link
Member

@marqh marqh commented Nov 1, 2016

usability tweak to CubeList.__str__ to print a STASH code, if it exists and otherwise
unknown / (unknown)
would otherwise be returned

@bjlittle bjlittle added this to the v1.11 milestone Nov 1, 2016
lib/iris/cube.py Outdated
units=self.units)
# If all unknown and a STASH attribute exists, use it.
if (nameunit == 'unknown / (unknown)' and
'STASH' in self.attributes):
Copy link
Member

Choose a reason for hiding this comment

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

This line can be joined to the above and it'll still only be 78 characters long.

Copy link
Member

Choose a reason for hiding this comment

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

And remember not to keep the parentheses.

lib/iris/cube.py Outdated
# If all unknown and a STASH attribute exists, use it.
if (nameunit == 'unknown / (unknown)' and
'STASH' in self.attributes):
nameunit = '{}'.format(self.attributes.get('STASH'))
Copy link
Member

Choose a reason for hiding this comment

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

self.attributes['STASH'] ... if you wanna, as you've already checked that the STASH key is available.

@bjlittle bjlittle self-assigned this Nov 1, 2016
@marqh
Copy link
Member Author

marqh commented Nov 1, 2016

minor changes, as per review comments

lib/iris/cube.py Outdated
nameunit = '{name} / ({units})'.format(name=self.name(),
units=self.units)
# If all unknown and a STASH attribute exists, use it.
if (nameunit == 'unknown / (unknown)' and 'STASH' in self.attributes):
Copy link
Member

Choose a reason for hiding this comment

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

No need for parentheses when on one line.

@ajdawson
Copy link
Member

ajdawson commented Nov 1, 2016

Does this affect how Cubes are printed too? I'm wary of removing a consistency like this, users expect name / (unit), and although this is not necessarily directly useful, it does flag that something is missing. This may not be the case if this is replaced by a stash code.

@marqh
Copy link
Member Author

marqh commented Nov 1, 2016

Does this affect how Cubes are printed too? I'm wary of removing a consistency like this, users expect name / (unit), and although this is not necessarily directly useful, it does flag that something is missing. This may not be the case if this is replaced by a stash code.

this doesn't change cube print cube.__str__, but it does change cube.__repr__

I think this is a reasonable change, as most code will use print and this will still say unknown / (unknown) and then print the attributes, including STASH if available

the aim is to improve the printing of cube lists, where there is unknown name and unknown unit

@ajdawson
Copy link
Member

ajdawson commented Nov 1, 2016

OK great, thanks for clarifying @marqh.

@ajdawson
Copy link
Member

ajdawson commented Nov 1, 2016

One last thing then: did you consider stash / (unknown) and reject in favour of just the STASH? I can see both sides of the argument, the STASH is not the name, so it may not be appropriate to follow the name / (unit) convention, but on the other hand, we still can't give the units, so should we include that part?

@marqh
Copy link
Member Author

marqh commented Nov 1, 2016

One last thing then: did you consider stash / (unknown) and reject in favour of just the STASH? I can see both sides of the argument, the STASH is not the name, so it may not be appropriate to follow the name / (unit) convention, but on the other hand, we still can't give the units, so should we include that part?

I did consider this; in principal we shouldn't have a name without a unit, so if we're falling back to STASH, we already don't have a unit

i do wonder whether stashmsi / (unknown) might be more specific, but it is longer, and i think it will always say unknown, as, if there is a unit, we'll print that rather than a STASH code

@ajdawson
Copy link
Member

ajdawson commented Nov 1, 2016

I guess the current strategy is fine, and really shines when someone knows what a STASH is and realises it probably tells them everything they need to know (or equips them with what they need to query with their data provider).

@bjlittle
Copy link
Member

bjlittle commented Nov 1, 2016

@marqh Did you consider something like stash=m01s00i004 rather than just the bare stash code (i.e. m01s00i004) being used instead of unknown / (unknown)

Would that be more meaningful/helpful to those users who are not stash savvy?

@ajdawson
Copy link
Member

ajdawson commented Nov 1, 2016

It would certainly make it clear that the presented information was different somehow from the normal presentation of name and unit.

@marqh
Copy link
Member Author

marqh commented Nov 2, 2016

Did you consider something like stash=m01s00i004 rather than just the bare stash code (i.e. m01s00i004) being used instead of unknown / (unknown)

I am content to do this, if you think it helps, though I don't like the = sign in the string

I'm keen to converge on one solution, so far we have

  1. m01s00i004
  2. m01s00i004 / (unknown)
  3. stash: m01s00i004
  4. stash=m01s00i004

please vote for your favorite or nominate another representation. I'll code up the preferred solution

@marqh
Copy link
Member Author

marqh commented Nov 2, 2016

please vote for your favorite or nominate another representation. I'll code up the preferred solution

@ajdawson @bjlittle ...

ty

@ajdawson
Copy link
Member

ajdawson commented Nov 2, 2016

I don't like 3, there are too many colons already in cubelist printouts. Number 2 is my next least favourite (I know, I brought this up...). I'd go with number 4, but if there are objections to this then my next choice is plain old number 1 (i.e. no change).

@bjlittle
Copy link
Member

bjlittle commented Nov 4, 2016

Apologies, I've been distracted ...

Okay, so I'm kinda in agreement with @ajdawson ... but I don't want this to be a thing, so old number 1 it is.

By golly I think we have a quorum!

@bjlittle bjlittle merged commit 8d223dd into SciTools:v1.11.x Nov 4, 2016
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