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

DM-6718 Add str and repr for afw catalog/record #256

Merged
merged 2 commits into from Jul 28, 2017
Merged

Conversation

parejkoj
Copy link
Contributor

@parejkoj parejkoj commented Jul 6, 2017

No description provided.

@parejkoj parejkoj changed the title DM-6718 Add str and repr for afw catalogs DM-6718 Add str and repr for afw catalog/record Jul 6, 2017
@PaulPrice
Copy link
Contributor

Great addition John, thanks!

from future.utils import with_metaclass

import numpy as np

from lsst.utils import continueClass, TemplateMeta
from .base import BaseRecord, BaseCatalog

__all__ = ["Catalog"]

Copy link
Contributor

Choose a reason for hiding this comment

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

This is in the wrong place, it should be moved back where it was. I too was bitten by this recently. Our coding guidelines state it should be at the top (to mimic pep8). My linter was still saying the top was not the right place, and it turns out I needed to update my linter to a newer version.

Copy link
Member

Choose a reason for hiding this comment

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

Is this something that's been announced to the LSST community? The last I'd heard about this issue I'd gotten the impression that it was the guidelines that were not PEP8-compliant (though I can see now that they are).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Yes, my flake8 needed to be upgraded.

if self.isContiguous():
return str(self.asAstropy())
else:
fields = ' '.join(x.field.getName() for x in self.schema)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the Astropy implementation print out rows (so you get something that looks like a table) and ours does not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this offline, but I'll make a response here, after talking it over with @SimonKrughoff :

We don't want to be constantly patching over the annoyances we have with our non-contiguous tables. We should instead fix them to behave better in general. So, instead of adding extra special casing to make non-contiguity "nicer" in each individual case, we'll do a "minimum functionality" thing and fix the non-contiguous tables themselves, by e.g. making numpy views into them that behave correctly.

@parejkoj parejkoj merged commit a69f52c into master Jul 28, 2017
@ktlim ktlim deleted the tickets/DM-6718 branch August 25, 2018 06:44
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

5 participants