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-7028: Port to Python 3 #8

Merged
merged 11 commits into from Aug 5, 2016
Merged

DM-7028: Port to Python 3 #8

merged 11 commits into from Aug 5, 2016

Conversation

timj
Copy link
Member

@timj timj commented Jul 29, 2016

No description provided.

# the GNU General Public License along with this program. If not,
#
# You should have received a copy of the LSST License Statement and
# the GNU General Public License along with this program. If not,
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit also includes PEP8 cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hoped you were going to let me get away with that one. My editor always strips spaces and I didn't catch this whitespace cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think RHL chastised me about that kind of editor setting several years ago.

* print function
* absolute import
@timj timj force-pushed the tickets/DM-7028 branch 6 times, most recently from d3b2a2e to 6e31c46 Compare August 4, 2016 22:22
This can work in both python2 and python3+futurize.
@timj timj force-pushed the tickets/DM-7028 branch 4 times, most recently from 2357f63 to ed7f34c Compare August 5, 2016 04:17

On Python 3 all ints are LongLong but we need to be able to store them
in Int containers if that is what is being used (testing for truncation).
Int is assumed to mean 32bit integer (2147483647 to -2147483648).
Copy link
Contributor

Choose a reason for hiding this comment

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

It may well be on most systems these days, but are we going to get bitten by assuming an architecture? Does PropertySet define it as a particular size, or just int ("at least 16 bits").

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to be int. @ktlim could we actually define this to use int32_t to make it more explicit?

minInt = -2147483648

# We do not want to convert bool to int so let the system work that
# out itself
Copy link
Contributor

Choose a reason for hiding this comment

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

How annoying that isinstance(True, numbers.Integral) is True!

Copy link
Member Author

Choose a reason for hiding this comment

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

My thoughts exactly when I saw the tests fail in afw last night.

Copy link
Contributor

Choose a reason for hiding this comment

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

You had to wait for afw to fail before you knew there was a problem here? Can you add a test that will catch the problem in this package?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to wait until obs_sdss for the integer overflow error to appear...

timj and others added 7 commits August 5, 2016 11:13
Python 3 does not distinguish long from int but PropertySet
and PropertyList can have different types. If something is added
to a container that already contains ints then assume Int, if
it already contains LongLong use LongLong. If the container
is empty choose based on the value.

We are careful not to include bool types in the guess work.

Adds some tests to ensure that boolean are handled properly
and that OverflowError is raised if the item is too large.
SWIG now returns int instead of long when the return value can
fit in an int.
@timj timj merged commit c7ec989 into master Aug 5, 2016
@ktlim ktlim deleted the tickets/DM-7028 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

2 participants