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

Add enclosing round bracket flags r and R to the format_num method #116

Closed
wants to merge 9 commits into from

Conversation

ep12
Copy link
Contributor

@ep12 ep12 commented Dec 6, 2019

Implement a new formatting option, see #113.
Note: This doesn't work with unumpy arrays like regular formatting does not work with them, formatting in general does not work with them (regular numpy's fault?).

@ep12 ep12 changed the title Add enclosing bracket flags b and B to the format_num method WIP: Add enclosing bracket flags b and B to the format_num method Dec 6, 2019
@ep12
Copy link
Contributor Author

ep12 commented Dec 7, 2019

Note: I tried to fix some python2 unicode related things that prevented the CI tests to pass. Only the first commit 053ae8e is related to #113!
Travis builds are now fine, but appveyor builds still fail:

ERROR: Test the formatting of numbers with uncertainty.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Miniconda3-x64\envs\uncty-env\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "C:\projects\uncertainties\build\lib\uncertainties\test_uncertainties.py", line 2088, in test_format
    % (value, representation, value_back)))
  File "C:\Miniconda3-x64\envs\uncty-env\lib\encodings\cp1252.py", line 19, in encode
    return codecs.charmap_encode(input,self.errors,encoding_table)[0]
UnicodeEncodeError: 'charmap' codec can't encode character '\u207b' in position 51: character maps to <undefined>

where \u2017b is the superscript minus. I changed quite a lot in different places, e. g. added u'' flags which helped a lot for Travis, but it also changed some return types in python2 from str to unicode. I am not sure if that is a problem.

@codecov-io
Copy link

codecov-io commented Dec 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@597c0d5). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #116   +/-   ##
=========================================
  Coverage          ?   94.88%           
=========================================
  Files             ?       12           
  Lines             ?     1995           
  Branches          ?        0           
=========================================
  Hits              ?     1893           
  Misses            ?      102           
  Partials          ?        0
Impacted Files Coverage Δ
uncertainties/core.py 97.54% <100%> (ø)
uncertainties/unumpy/core.py 95.54% <100%> (ø)
uncertainties/test_uncertainties.py 96.28% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 597c0d5...4ff064f. Read the comment docs.

@ep12
Copy link
Contributor Author

ep12 commented Dec 7, 2019

I just noted that I reverted quite a log of changes from 26dafcc, where e. g. the u'' flags were removed.

@ep12
Copy link
Contributor Author

ep12 commented Dec 7, 2019

Hooray! All tests have passed.... Time to add new tests for the b and B flags...

@ep12 ep12 changed the title WIP: Add enclosing bracket flags b and B to the format_num method Add enclosing bracket flags b and B to the format_num method Dec 7, 2019
@ep12 ep12 mentioned this pull request Dec 7, 2019
@ep12 ep12 changed the title Add enclosing bracket flags b and B to the format_num method Add enclosing round bracket flags r and R to the format_num method Dec 22, 2019
@ep12
Copy link
Contributor Author

ep12 commented Dec 22, 2019

AppVeyor fails with the message error: invalid command 'nosetests'...

@CAM-Gerlach
Copy link

CAM-Gerlach commented Mar 5, 2020

@ep12 As an outsider considering this project for building our scientific IoT sensor framework (thanks to its integration with pint) and hoping to contribute myself (but strongly put off by the archaic code base and apparent lack of real maintenance), I'm not sure why it failed as other more recent builds have at least managed to run the tests, and I don't see anything obvious in the code that would prompt it.

To note, I'm unfamiliar with the particulars of the ancient syntax and infrastructure used (<Py2.7, nose, 2to3 for on-the-fly conversion, easy_install-style setup.py, etc) since that stuff has been deprecated for as long as I've been a scientific programmer (half a decade now)...heck, the only reason I even have indirect exposure to 2.7 is due to helping other people port their code to Python 3, and maintaining 2.7 backward-compat up till now as a core dev of the Spyder scientific environment.

However, random CI failures are unfortunately something I'm all too familiar with. If you rebase on the latest master and push to retrigger a build it may resolve itself, either due to picking up a upstream change or simply due to a transient issue.

@lebigot Any help here? This needs to be merged to fix your tests, which have been failing for months because of it. Thanks.

@lebigot
Copy link
Collaborator

lebigot commented Mar 5, 2020

The "archaic" code base exists for the purpose of supporting scientific experiments that rely on older versions of Python, but I did agree to stop supporting Python <2.7 and to move to Python 3 as the main source code version.

I am not sure what you mean by the "apparent lack of real maintenance": I don't recall any bug being ever found in this package, so there is no need for maintenance when something works, right?

I understand that the build fails because of the packaging introduced in a relatively recent pull request merge. I am looking into this when my baby lets me. I am basically revising the way wheels are built.

@lebigot
Copy link
Collaborator

lebigot commented Mar 5, 2020

In practical terms, you may want to base your changes from the last version that builds (e3b8f71).

@CAM-Gerlach
Copy link

CAM-Gerlach commented Mar 5, 2020

Sorry if I came off a little harsh...I'm just so used to the "modern" way of doing things since I haven't been around long enough to have learned the old way. Unfortunately, as I'm sure you're well familiar with, our current funding and advancement models in academia and research just isn't set up very well to support vital maintenance on core infrastructure packages like what you're doing despite how important the work you do here is to the scientific community, so between that and your family and all the demands for new features I can't imagine things have been easy for you.

If you'd like some help moving to a more modern and easier to maintain Py3 codebase, I may be able to help out, although for the very near term at least I'm absolutely stuffed between building an IoT framework for 2 different scientific sensor systems, doing a ML/AI project for NASA, working on a project for the Spyder scientific environment, and running an outreach thing with a fleet of boats down at Cape Canaveral/NASA KSC to help people watch rocket launches. Possibly in a month or two depending on your timeframe...let me know if I can assist.

Sorry for derailing this issue...feel free to follow up via other contact methods.

@lebigot
Copy link
Collaborator

lebigot commented Mar 5, 2020

No offense taken. :) Thanks for the offer. There is a pull request with a move to Python 3, but it contains many other changes, changes that the official Python documentation does not agree with, and also non-atomic commits that are hard to reuse, so after handling the wheel building, and probably a smaller pull request, I'll be moving to Python 3 and a universal source code.
In any case, good luck with all your projects!

@CAM-Gerlach
Copy link

There is a pull request with a move to Python 3, but it contains many other changes

It might be good to mention your concerns over there to give the contributor a chance to address them, as it doesn't sound like they are fully aware of them. While I agree that smaller, more incremental PRs would be ideal, in cases like these with so much legacy code one runs into a "chicken or the egg" (er, wheel) problem when there is a substantial amount of interdependency between all the elements you mention.

More specifically, up to date versions of both pylint and pytest require Python 3, for which future is the current officially recommended porting tool; conversely, using pylint, Tox, and a modern non-deprecated, fully Python 3-supporting test framework (pytest, as nose is none of these things) are integral parts of the porting process, all per the official Py3 Porting Guide in the Python Docs and many other similar authoritative resources. Eggs->Wheels isn't directly linked and could have been done as a precusor, although they have deprecated nearly a decade ago, were never an officially supported Python standard and do not support universal (Python 2 + Python 3) packages.

changes that the official Python documentation does not agree with

I'm not sure I'm clear on which those would be; I spent time time looking through the PR in quite some detail but while I found a smattering of minor issues and things that could be done even better (which I'd be happy to point out in a review if the PR has a good chance of acceptance), few to none were an actual regression over the current situation, and I overall found it to be a net improvement in almost every major respect in terms of implementing best practices, conforming to accepted standards, and following the letter and the spirit of the relevant authoritative documentation.

and also non-atomic commits that are hard to reuse

I don't really know about re-use being an independently vital property since if you mean like in cherrypicking that's not really considered an integral part of the core VCS workflow, but atomicity absolutely and overall, I definitely agree that the commit structure is by far the weakest part of that PR. I would ask that the contributor rebase it and reoganize the commits to split the work into discrete functional changes, and be more balanced with the commits, but while this is far from ideal, I personally wouldn't consider flat-out rejecting a PR based primarily on just its commit structure, as the changes still add much more positive and needed benefit than the commit structure takes away overall.

@ep12
Copy link
Contributor Author

ep12 commented Mar 21, 2020

In practical terms, you may want to base your changes from the last version that builds (e3b8f71).

I tried rebasing the stuff and noted that my work was based on 597c0d5 (newer than e3b8f71).
Note that 81a9594 (above) was my last commit where all tests passed. That changed with my next commit, 4ff064f.
However, merge conflicts appeared that had nothing to do with what I have done and I don't want this feature implementation to ruin your work to make the builds and tests pass.
Since the implementation of the feature itself is rather trivial, I think it is better to first fix the builds and tests and then I will reimplement this feature without having to do unrelated bug fixes to get the pr tests to pass.
I'll close this pr but I'll leave the issue #113 open as a reminder.

@ep12 ep12 closed this Mar 21, 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