Development Plans

[newville]

With the transfer from lebigot/uncertainties, to lmfit/uncertainties, we have promised to maintain, update, develop, and support this library. The discussion at #180 gives several ideas for what to do in the near-term and long-term. I propose that we write those down here, and try to set priorities for the next 1 to 6 months. I'll start with my opinions, but happy to hear others

I think we should aim for a release around April 1 to April 15 with approximately the following "near term goals":

Packaging, Testing, and Deployment

1. Support Python 3.8 to 3.12
2. use pyproject.toml and more modern setuptools
3. use setuptools_scm for version
4. move test code to tests folder, run with pytest there.
5. migrate CI to either GitHub Actions or Azure.
6. migrate docs to either GitHub Pages or ReadTheDocs.

Code:

1. remove Python2 code
2. move to mostly f-strings for string formatting (if nothing else, requiring Python 3.6).
3. move tests to a separate tests folder.
4. maybe: interchange "UFloat" with "AffineScalarFunc". That is, currently the code uses "AffineScalarFunc", and "UFloat" is an alias.

I think "update numpy interface" and "formatting schemes" can come later and/or need more discussion.
That gives a few questions:

1. is this generally ok?
2. preferences for GitHub Actions v. Azure v. other. I would probably go with GitHub Actions myself, happy to defer.
3. preferences for GitHub Pages v. ReadTheDocs v. other. I would probably go with GitHub Pages myself, happy to defer.
4. is moving the tests and using pytest ok?
5. any other urgent ideas or concerns that we should delay some of these for further discussion?

Really, happy to have the discussion and to defer to others on these.

[lebigot]

Thank you @newville.

What exactly do you mean by "migrating the docs to… ReadTheDocs"? The documentation already lives there…

Also, maybe it's also a good time to add some developer documentation while you guys create the new setup? This is something I never really put in the documentation, but maybe some contributors struggled a bit with how to get started with tests, etc.?

[andrewgsavage]

With the exception of

maybe: interchange "UFloat" with "AffineScalarFunc". That is, currently the code uses "AffineScalarFunc", and "UFloat" is an alias.

your proposed goals don't affect users but make it easier to develop and maintain. Is that intentional? That does sound like a good goal for a first release.

Could we try to discuss distinct issues in seperate issue/discussion topics? The last one got quite big!

1. yes
2. no preference
3. preference for RTD. Could use a seperate issue/discussion for documentation improvements
4. Yes, that would help a lot. May be worth a seperate issue/discussion for how we intend to add/migrate tests using pytest.
5. I'll add 'reorganise core into smaller files' to your later/more discussion list.

What's the intention with privileges? I don't have merge privileges and it looks like that's stopping @jagerber48
working on #191 . (I don't need merge privileges, just raising it as something that needs doing.)

[wshanks]

Regarding privileges, I have the ability to merge, but I think that was carried over from what @lebigot gave me before the transition. I don't have the ability to change repository settings like enabling GitHub Actions. Maybe something is missing with the settings for the "Uncertainties Developers" team, @newville? One thing that is strange to me is that on https://github.com/orgs/lmfit/teams, it only shows two teams to me:

But if I click on "People" in the organization and then click on the "1 team" link for myself it shows me the "Uncertainties Developers" team. I am not sure why it is separated from the other teams like that:

[wshanks]

I am in favor of all the suggestions above. A few other ones that do not affect users much:

1. Remove the 1to2 code for helping migrate from uncertainties 1 to uncertainties 2.
2. Add some basic linting to CI and consider using a code formatter as well.
3. If adding linting/formatting, also add a config for pre-commit that calls the linter/formatter in the expected way.
4. Maybe add a config for a project management tool like tox/nox/hatch/make for quick access to the commands for running the tests and building the documentation.

[newville]

Hi,
on docs location: sorry, I missed that these are on readthedocs already -- that's a fine place and there is no reason to move them.

On privileges: sorry, I think I am still learning this. I made everyone here an owner -- it would be nice to have finer-grained control, but that's OK.

@wshanks

your proposed goals don't affect users but make it easier to develop and maintain. Is that intentional? That does sound like a good goal for a first release

Yes, exactly. I suggest we first clean up and get the code to a state that we are comfortable working on it.

+1 on removing uncertainties 1to2 code too.

I am OK with adding pre-commit hooks. I am not super familiar with tox/nox/hatch, but am OK with using those too.

Could we try to discuss distinct issues in seperate issue/discussion topics? The last one got quite big!

On "AffineScalarFunc" to "UFloat": I think it is not that big of a change, but maybe a real discussion point.

Currently, Variable subclasses AffineScalarFunc, and there is
UFloat = AffineScalarFunc

That means that some messages claim the object created by
u = ufloat(1, 0.3)

is a Variable (fine), but some refer to it as "AffineScalarFunc" or "affine function". That's not wrong, but slightly confusing to some (see #186 which is not really about that confusion but does show it).

I think it is more work (and maybe not desirable) to just fold everything in Variable. But if we rename AffineScalarFunc to UFloat and swap the aliasing to be

AffineScalarFunc = UFloat

then I think that nothing breaks, but the messages might change to something slightly more comprehensible.

But it's just a suggestion: I am totally fine with shelving this until later.

[lebigot]

@newville: moving the AffineScalarFunc code to UFloat and doing AffineScalarFunc = UFloat should indeed not break anything (they are the same class). This should only change the name of the class (to UFloat).

Now, I haven't looked at the code, but I doubt that the code in years, for UFloat/AffineScalarFund could (or should) be folded into Variable: Variable represents an independent variable, while AffineScalarFunc represents an affine scalar function of multiple variables.

[wshanks]

@newville Regarding finer grained control, I added the Uncertainties Developers team as having admin privileges in the uncertainties repository settings. If you want, you can change my organization role from owner back to what it was before and I can confirm if I still access to the repository settings. Also, I realized why I couldn't see Uncertainties Developers in my screenshot above -- it is a subteam of the Developers team. Also, one note -- I don't know if it breaks the way you were trying to model privileges to give a team called "Developers" admin on the uncertainties repo. Maybe it needs to be called "Uncertainties Admin" to be more clear.

[newville]

@wshanks Thanks - it might very well be that I have no idea what I'm doing with GitHub Teams, or am just now starting to understand them ;).

I have now:

1. successfully created a top-level "uncertainties admins" team
2. added @wshanks @andrewgsavage @lebigot @jagerber48 and myself.
3. added lmfit/uncertainties to this team and gave everyone on that team "Admin" permissions.

I left you as an owner of lmfit, because we clearly need a few people who know what the heck they're doing ;)

[jagerber48]

Well I know github actions and have created a PR for it already #194. I don't know Azure CI. If there's a reason to move to Azure that's fine, but it feels like "an additional dependency".

Regarding UFloat/AffineScalarFunc. I need to look at the code to see exactly what is being discussed. But it sounds like we're imaging maybe version 3.2.0 instead of version 4.0.0 for this first release. If that's the case I don't think we should introduce the name UFloat yet. That is, I think it will be more "clean" for everyone if the bulk of the major API changes are introduced at 4.0.0. But maybe this suggestion is to swap the role of ufloat and AffineScalarFunc. I think doing some of that backend work to make some error messages or other messages more clear is in scope for a 3.x release.

Looks like docs are already on readthedocs. I guess we may also somehow need shared access to that?

[lebigot]

UFloat already exists (nothing will be introduced). The idea is only to hide AffineScalarFunc as much as possible from users, because it's an implementation detail.

Also, I guess you meant swapping the roles of UFloat and AffineScalarFunc, not ufloat, which is a factory function and not a class?

[newville]

Thanks, yes, that is exactly what I meant: swap UFloat and AffineScalarFunc with the only intention of hiding AffineScalarFunc a little more. The function ufloat would remain unchanged, returning a Variable, which would then be a subclass of UFloat.

[jagerber48]

Suggestion that a changelog is added with somewhat high priority so that we can keep track of changes as we go? I try to follow Keep a Changelog conventions. I guess it's a discussion topic whether a changelog should keep track of backend changes like a lot of what we're working on now, or if only user-facing changes should be documented. I tend to prefer the former but could easily be convinced otherwise.

[wshanks]

I like changelogs as well. Some other projects I work on use reno to create notes as individual files with PRs to avoid merge conflicts on one shared changelog file. I think towncrier might be more popular for this than reno for Python projects.

[jagerber48]

Yes, I've seen towncrier but never used it. I'll look into (and reno) and see what I think.

[wshanks]

One thing we haven't discussed is merge policy. Should we aim for approval by one maintainer for merging PRs? #194 is adding GitHub Actions runs of the tests. Shall we also set that as required for merging into master?

Also, I generally prefer squash merging PRs into the main branch to keep the commit history clean. Do others have a feeling on this? I also respect rebase merging a series of logically separate commits but GitHub doesn't make that style easy to review because its diff view works best for viewing new commits, not editing old commits, so it encourages responding to PR feedback with new small edit commits and those I prefer to get squashed down.

[jagerber48]

Idea (I don't know if it's a good one): In this initial period where we anticipate a lot of work and we're all new to carrying a larger responsibility in this repo we could target approval by two maintainers for PRs. Then as we get used to working on this package, as we get to know each other, and after version 4.0 we have a plan to reduce that burden down to 1 approval by a maintainer? Would requiring 2 maintainer approvals slow things down too much?

I think passing CI should be a requirement for any PR to be merged into master. We should hone CI so that it isn't too easy or too hard to pass so that it is a nice relevant test for merging PRs.

For merges: I'm not opposed to seeing the nasty history, but I wouldn't object if people prefer squash commits. I've never worked much with rebasing and don't see the advantage over merge, so I'd prefer a merge policy of rebase.

[andrewgsavage]

If I remember correctly, the pandas UncertaintyArray was pretty much ready. I think it could use a Warning on initialisation so we're free-er to move it around or rename it as needed.
UncertaintyArray is currently experimental, and its API may change without warning.
Also need to set the tests to run with pytest

It would be good to get it into a release and see where it breaks for people.

[willynilly]

I am interested in helping with this project. I noticed that the code currently lacks type hints. What do you think about adding type hints to the code?

[jagerber48]

@newville

Duck-typing can have some very real benefits including code compactness.

Yeah, I think you hit the nail on the head with the main downside of type-hinting. Type hinting (from my somewhat naive perspective) seems to be a bit at odds with duck typing.

I would request a description (possibly in a separate discussion topic) describing what changes would be made and giving some justification for adding the proposed type hinting.

This makes sense.

Why is passing a type checker a goal? I'm not saying it isn't, I just don't know why it is.

The main reason I personally like type hinting is because I appreciate my IDE showing me they type hints when I'm writing code. Seeing the type hints is just a little mental nudge helping me know exactly what I need to do when I'm writing a function or instantiating a class or something. The other main reason I like type hints/passing a type checker is I find that, in my opinion, the process of refactoring code to pass a type checker usually qualitatively improves the code. The interfaces and functionality ends up a little bit tighter and more intentional.

Like, are there known Issues that type-hinting would fix?

No, I doubt there's an issue type-hinting would fix. I think it is total personal preference.

I would probably advise against starting with adding type hinting to the tests. It seems to me that this is precisely the place where you want to allow for and test the reaction of the code to various types of input. We are trying to maintain and modernize a fairly mature code base. Which tests need type hinting added?

I agree with this. I think tests sound like a strange place to start off adding type hints

---

I think type hinting is very much a personal preference thing. I think some people prefer it and others don't. I think with or without type hints are both 100% valid.

---

My personal feeling is that adding or even discussing the option of adding type hinting should be at the bottom of our priorities list. I think we should implement all the other higher priority changes that we have. Then once we feel the library has re-stabilized, it would be pretty straightforward to make a PR that would give a nice with/without type hinting side-by-side comparison and we could have a concrete discussion. Adding type hinting now would probably result in unnecessary work that will just get deleted or obsoleted as other changes are made. If we unanimously agreed that we wanted to do type hinting it would be fine to work on in piece by piece as we make progress, but we don't have that, and I think it's low enough priority that we don't want to spend effort trying to get agreement on type hinting philosophy.

TLDR: My opinion is that I prefer type hinting, but I think it makes sense to kick the type-hinting can down the road until we've tackled many of the higher priority line items.

[wshanks]

Having a code base that passes strict type checking takes a lot of care. I have done it for projects that I started from scratch but not for a pre-existing project. I agree with @jagerber48 that it can lead to better code (by some perspective) but for a code base that is already loose with types the annotations can get unwieldy and there is temptation to change the code behavior (which we want to be cautious about with this old and established project).

Thinking less strictly, I think type annotations are nice form of documentation. I prefer them to having the types in the docstrings. Aiding editor coding hints is also nice.

So I would be open to adding type hints, but I have some reservations about them getting too complex and would want a little more study before saying for sure. It might be instructive to open a PR adding type annotations to a few of the functions included in the __all__ of uncertainties.core to see how controversial they are. The tests themselves do not need type hints since they accept no arguments and return no values, but there is a set of helper functions in the tests and perhaps adding type hints to those would be a lower risk place to start if @willynilly preferred that to the main code (that would avoid conflicts if anyone else has started any further refactoring of the main code).

[willynilly]

I found this article, which talks about the pros/cons of duck typing, and how protocols (a Python 3.8 feature) are used to retain the advantages of duck typing and gain the advantages of type hints: https://realpython.com/duck-typing-python/

[willynilly]

In many tests, I use type hints, since type-related bugs can be introduced in tests as well, and type hints can make explicit the intended scope of the test. I also recognize that one might want to test how a function behaves when provided with arguments of the wrong type (or more generally, lacking the wrong interface as with ducktyping). This is also possible to do without being blocked by an IDE's type checker. Your test can declare a variable of the wrong type (using a type hint), then use this variable as an argument to the function you are testing, and then add a special type hint ignore comment on the line where the function is called in your test. This allows the test to take advantage of the type hint in the declaration of the function argument but avoid the typechecker blocking the test when it encounters the function call.

[lebigot]

I concur with all of the above facts (not necessarily personal preferences).

If I can share my opinion: it's essentially that of @newville, with the added point that type annotations make reading argument lists quite cumbersome—but at the same time I agree that type annotations that help IDEs are convenient for users, so I wouldn't mind user-facing functions and methods to have type annotations.

[andrewgsavage]

type hints were suggested for a future release. PRs are welcome

[andrewgsavage]

I was not think and pushed to master, what's the best way to revert this?

I did get this error message which I thought would prevent it merging:

$ git push
Enumerating objects: 15, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 12 threads
Compressing objects: 100% (8/8), done.
Writing objects: 100% (8/8), 768 bytes | 768.00 KiB/s, done.
Total 8 (delta 7), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
remote: This repository moved. Please use the new location:
remote:   [email protected]:lmfit/uncertainties.git
remote: Bypassed rule violations for refs/heads/master:
remote:
remote: - Changes must be made through a pull request.
remote:
remote: - Required status check "Final Results" is expected.
remote:
To github.com:lebigot/uncertainties.git
   e9b82f0..aab68cf  master -> master

[jagerber48]

@andrewgsavage I don't know how that happened. It looks like all the branch protections are on such that no one, not even administrators, can push directly to master. I think a "revert commit" is the way to go. But I'm not a git expert when it comes to correcting these kinds of things..

I wonder if it's actually some sort of bug having to do with the fact that you pushed to lebigot/ instead of lmfit/. Like branch protections weren't enabled for lebigot/ so the push somehow snuck through that way... Just a conspiracy theory.

[wshanks]

There is an extra "Do not allow bypassing the above settings" option that was not selected and apparently exempts administrators from the protection rules. I just enabled the setting. You might have noticed that I momentarily pushed an empty commit to master just now and then rolled it back while testing the protection rules.

[newville]

@andrewgsavage @jagerber48

I did a git revert HEAD~1 followed by git push. Should be back to the previous commit, even if it shows up in the history...

[jagerber48]

@newville looks like you reverted the wrong commit? We wanted to revert 4f26 authored by Andrew but it looks like you reverted the earlier commit e9b8 authored by Will Shanks. Maybe you forgot git fetch before you revert?

I don't know how it is people are able to push directly to master. It's a bit concerning.

[newville]

@jagerber48 @andrewgsavage Um, yeah, I did a git revert... which is apparently not what we wanted.

OK, now I did

git reset --hard e9b82f045f423452cfaeddc0b
git push -f origin

For this push -f to work, I did have to disable branch protection and then I re-enabled it. I do not know why the non-forced pushes were allowed...

[jagerber48]

Looks good to me now

[andrewgsavage]

great thanks

[andrewgsavage]

Most of the things listed here for the first release has been done. Are there any objections to a release after #211 #214 are merged?

Packaging, Testing, and Deployment

• Support Python 3.8 to 3.12
• use pyproject.toml and more modern setuptools
• use setuptools_scm for version
• move test code to tests folder, run with pytest there.
• migrate CI to either GitHub Actions or Azure.
• migrate docs to either GitHub Pages or ReadTheDocs. (already on RTD)

Code:

• remove Python2 code
• move to mostly f-strings for string formatting (if nothing else, requiring Python 3.6).
• move tests to a separate tests folder.
• maybe: interchange "UFloat" with "AffineScalarFunc". That is, currently the code uses "AffineScalarFunc", and "UFloat" is an alias. (leave till future release + more discussion)

wshank's list

• Remove the 1to2 code for helping migrate from uncertainties 1 to uncertainties 2.
• Add some basic linting to CI and consider using a code formatter as well.
• If adding linting/formatting, also add a config for pre-commit that calls the linter/formatter in the expected way.
• Maybe add a config for a project management tool like tox/nox/hatch/make for quick access to the commands for running the tests and building the documentation.

[andrewgsavage]

Oh there's actually a bit more remaining than I thought. Should probably wait on setuptools_scm.

Removing python2 code is quite time involved and I think setting up the linter may help with it. I'd be happy to get a release out without a linter/removing all python2 code.

[wshanks]

Looking at the list though, there is not too much exciting for users. I think the user facing changes are:

1. Drop Python 2 support and official Python <3.8 support.
2. Remove some deprecated deprecated API functions/methods and allowed argument formats.
3. Remove the tests from the installed package.
4. Drop the dependence on future.

I am not opposed to a new release though. Testing out the releasing process would be good to do, and a new version number showing up for users would signal more widely that the project has new life. Also, making the release a little smaller / not depend on future is nice.

[wshanks]

Also, I meant to say that none of the remaining things on the list are user facing, so they shouldn't block a release, I think. A small argument for trying to get to them before release is that the release could bring attention from more contributors and some of the things on the list help with onboarding new contributors.

[andrewgsavage]

I had a look at setting up pre-commit and ruff. Activating

    -   id: check-yaml
    -   id: end-of-file-fixer
    -   id: trailing-whitespace

made changes to many files. I think this should wait until after a release and any PRs waiting for after the release, like #198 are merged.

[wshanks]

That sounds reasonable. We will need to look over the PRs and decide when a good time to do it is. There will be one point of pain somewhere, I think. I would hope running an automated change like trailing-whitespace on master and a PR branch makes the merge not too bad but with a large change like #198 it would be painful.

[lebigot]

About having the documentation on readthedocs: maybe some confusion arose from some old documentation living at https://pythonhosted.org/uncertainties/. It should be removed (I'm not sure right now how to do it).