-
Notifications
You must be signed in to change notification settings - Fork 36
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
sciform
review
#121
Comments
Linking back to discussion on the presubmission issue: We will hold off on starting this review for just a bit (two weeks maximum) while we help @jagerber48 with some questions about development and scope. |
I have come to decisions on most of the questions I had that @NickleDave referred to in the previous comment. For one question I opened a topic in the pyopensci discourse: https://pyopensci.discourse.group/t/how-to-avoid-repeating-a-long-list-of-keyword-options-throughout-a-package/331/5. The suggestions there are very good and I plan to explore them in upcoming tests and versions for @NickleDave how do you suggest we proceed with the review? I'm in an active phase of development so I'm not sure what makes sense in terms of hitting a moving target. I just made an important update in version I don't have any rush to get this review going if we want to wait for more recent code on the one hand. But on the other hand, if I keep changing the code and not starting the review we'll never hit the moving target.. |
Hi @jagerber48 thank you for updating here. You are right that we should not let this get to a place where you keep changing the code. Please release that version, which you expect to be 0.23.0, and then set that as the version submitted above. |
Thanks. That makes the most sense to me also. That's what I'll do. |
Great, thank you @jagerber48 -- once you reply back that you've released that version, we'll go ahead with the review |
Ok. I've implemented a version of the changes discussed in the discourse topic. I'll post details in the topic since progress has been made but there may still be room for improvement. I may continue to make small code/docs changes and releases during the review but I'm happy to pin the review to version |
I pushed version Some general questions I have that may be in-scope for the review:
Then I'll also mention that I've found making the documentation good and consistent has been my biggest challenge/time spent on this project. I continue to have ideas for documentation improvements but documentation has been a moving target as the code has continued to change. Just to say: I expect errors in the documentation and places I can make improvements. |
I don't have any non-trivial breaking changes in mind for the package at this time (though stuff always seems to come up). The only API changes I have in mind are possible name changes to options or classes. There are a few I'm not 100% happy with so I may poll pyopensci or e.g. code review stack exchange to try to get some broader set of opinions on some naming choices. Just two I've written down:
Those are just two I had written down, but pretty much any name in the API is fair game for naming improvement suggestions. |
Thank you for letting us know @jagerber48. |
|
Hi @jagerber48, As we discussed, I will lead the review for |
Here's an update on the current status of the package and some questions. Since making this submission I modified the code quite a bit moving up to version
For adding the non-core features I plan to wait until after the pyopensci review and possibly until after I release version 1.0.0 with a more stable API (which will also happen after the pyopensci review).
These are some of the main existing api features that I have questions about. I.e. making decisions on the above will constitute breaking changes, so I want to address these before 1.0.0. I am ALSO open to feature requests but will be implementing those with lower priority than stabilizing the API. Thank you so much for taking the time to read this and also thank you very much for any feedback you are able to provide! I'm also curious to hear suggestions for other communities where I could seek feedback. |
👋 Hi @isabelizimm and @machow! Thank you for volunteering to review for pyOpenSci. I am truly excited by the team we have on this review. @jagerber48, Isabel and Michael are involved with quarto, and I believe they have some pretty good opinions on how to format numbers for publishing. I am looking forward to seeing their review! Please fill out our pre-review surveyBefore beginning your review, please fill out our pre-review survey. This helps us improve all aspects of our review and better understand our community. No personal data will be shared from this survey - it will only be used in an aggregated format by our Executive Director to improve our processes and programs.
The following resources will help you complete your review:
Please get in touch with any questions or concerns! Your review is due: 27 October 2023.Reviewers: @isabelizimm, @machow |
Started running the code on real applications myself some more and quickly found some important bugs. Just made a release of version |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Has examples page
Readme file requirements
The README should include, from top to bottom:
NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)
UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 4-5 Review CommentsOverall, this package is super nifty and clearly offers a huge quality of life increase in scientific publishing. The documentation, as a whole, is pretty comprehensive with lots of examples and a mix of API and narrative style docs. Thank you for a great addition to pyOpenSci!
Response to a few maintainer questionsI've answered a handful of your questions that haven't had a response yet + I have a response to, but happy to have discussion on any other points you're interested in chatting about! 😄
I think setting
I wonder if you could somehow work padding into the name? My thought process behind that:
I have a slight preference to
For
I like SciNum.
It was not immediately apparent to me that
For binary formatting, maybe open an issue/discussion about it and ask people to 👍 if they would like to see it implemented? That way you're not building things you don't have interest in. However, if you already have the plumbing in place, and it's just a matter of exposing an argument, and you feel compelled to do so, then that seems like a good interrim move!
I think yes! For every release, it is super useful to see the changes. If it feels like too much effort to go back and write everything manually, even something small like using the
Something you can do locally is install and use Optional nits:
|
Thank you kindly for this thorough review! |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Readme file requirements
The README should include, from top to bottom:
NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)
UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Functionality
For packages also submitting to JOSS
Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted. The package contains a
Final approval (post-review)
Estimated hours spent reviewing: 2.5 Review Comments
I think that--as the package maintainer--if bundling multiple features into a PR is working for you, then it seems okay. It seems more important to limit the scope of PRs to other people's repositories, so that 1 feature doesn't get held up waiting for feedback / discussion on others.
You can use the github action https://github.com/pypa/gh-action-pypi-publish to publish to PyPI whenever you make a github release. Here's an example in a tool I maintain called quartodoc, in case it's useful!
You can use tox, though I just push to github and let the github actions run the tests. There's also act, for running github actions locally (though I've never used it).
I have no good opinions on the exact names, but like that they use camel case, so feel more class-y.
This seems super handy!
Tools like pydantic can take Literals or Enums, and give nice validation / feedback if an invalid value is passed. If pydantic feels too heavy duty, I like the suggestion of
If Formatter can only be initialized using a single
Not helpful opinions here :/. I think that the package is so useful for this kind of problem that I'd be motivated as a user to figure out what all the names meant, though!
The FSML seems really neat, but reading through the documentation / trying the package, it feels easier to specify FormatOptions (and with literals or some approach that makes seeing options in autocomplete / code completion in eg VS Code would be very fast to initialize!). I always have to look up the options in the built-in string formatting mini-language, but initializing a class gives a lot of nice hinting / auto complete options for helping folks. (I could see the FSML being nice for quick, common formatting though!) Overall, it seems like a really helpful library! |
Thanks a bunch, @isabelizimm and @machow, for the reviews! @jagerber48 I also took a few notes (without the suitable format, editor's privilege 🐈⬛ ) Project documentationREADMEInstructions are clear, and the statement of need as well.
ContributingMissing as of now. LicenseOk. ChangelogOk. InstallationInstallation resolves correctly. pyproject.tomlThe classifiers are lacking (python version).
General remarks and QoC
|
I'm going to rename Here's an idea on |
re: development docs: I reviewed that PR, a few small comments but looks good to me! It does really help users to get started contributing when they don't have to guess about some of the going-though-the-motions pieces of development, so I think this is a great addition. Plus, everything ran on my laptop right out of the box, which is always exciting➕ 😄 re: CI/pre-commit: I personally don't use act/tox/etc. I've tried a few, but no project of mine has been heavyweight enough that I found it a net-win to my workflow. The one thing I do use, though, is pre-commit hooks. It might be nice to start using this tool locally to make sure all new code is compliant, and integrate it into CI as you see fit. A few pre-commit moves that might make it more useful to you:
re: naming: These both seem like good moves! The digits place -> decimal place move immediately clicked in my brain, and having uncertainty as a parameter seems reasonable enough to me. |
@isabelizimm ok yes, Ok I'm working on another release (
Anyways, if any of you are inclined, @Batalex @machow @isabelizimm I would appreciate if any of you have comments on this jump from There are a couple improvements I see that can be made, but I consider them to be typical package improvements that should happen over time and I don't know that they have bearing on the review.
|
IMO we are at the end of the review process. Thank you kindly for bearing with our demands! Before I ask @machow and @isabelizimm for their final approval, I would like to ask you if you feel that the API is stable enough so that you can release 1.0 in the same time frame as the pyopensci "seal of quality"? This is not an obligation by any mean, of course. |
@Batalex great! Thank you! I've been asking myself the
|
I just released version My current action is compiling my |
Ok, I got my thoughts collected. Previously I had to do lists in various places. Now most of the todos are captured as tagged (labeled) issues. Most of the issues get the However, two of the issues got tagged for the Release 1.0.0 milestone. They both correspond to what I would consider to be either strange or broken behavior for a couple of options. I'll just link the issues here. I would appreciate any thoughts on the issues that anyone here has.
The latter one especially might result in, e.g. removing the global settings functions like The resolution to this issue May also constitute a small breaking change to the behavior. Regarding naming: I'm pretty happy with where things are at at the moment. There's maybe a couple names I don't love (e.g. I do have some internal debates about what My feeling is that given (1) @Batalex @isabelizimm @machow curious for your feedback on
|
Addressed the 3 issues linked in the previous post to release 0.32.0. That's all the concrete stuff I had on my list before releasing I capture my thoughts on releasing |
Regardless of when you bump to v1, I'd definitely +1 advertising it broadly to folks! I'd think about it like this: if you bump to v1 without people using it, you have to guess what the right behaviors / naming is. If you can get people using it, even with the caveat it's v0, you'll have good feedback on potential changes for a v1. Because v0 explicitly signals you might make changes, in a way you're in a good position to advertise it AND think about important tweaks! |
Yes, this is exactly what I'm thinking! I don't want to have to guess at right behaviors / naming which is what it feels like I would be doing if I bumped to v1. I don't think v0 is at all an impediment to getting people to use it. That is, I think the fractional intersection of people who might be interested in So if that's really true maybe I should just cool my horses on v1 and instead commit to something like (and indicate this in the docs) "I'm going to use and advertise |
Before going into my thoughts/questions about advertising/getting feedback: What is the status of the pyopensci review at this point? I greatly appreciate the continued feedback I'm getting but also don't want to veer the conversation out of scope or off topic. @Batalex? My rough thinking on Suggestions for advertising
How do these ideas look? Where else can/should open source packages be announced? Further thoughts on the python discourse post: I think it would be inappropriate to revive that thread. I just looked through it a little bit. It was originally about adding SI/IEC prefixes into the built in FSML and then it kind of got hijacked (partly by me) into talking about engineering notation and sig figs. I think the thread does clearly discover where the python FSML falls short with respect to sig figs. The thread is originally about adding prefixes, and the OPs package |
Another breaking variable rename recently occurred to me while I was answering a stack overflow question
This would be much more consistent with its option partners |
Hey @jagerber48, @machow, @isabelizimm I need you to check
in your reviews above. As for how to advertise
|
Hey there-- I have given a ✅ on my review. The plans and notes above for advertising seem like a solid start; if you're involved in other Python discord/slack/etcs, there's often a place to show off work. ++ on v1 being a personal choice, but that it is never too early to solicit feedback! This package is in a solid state, and @jagerber48 should be really excited and proud of building it up! |
To be clear, yes, I don't want to wait for 1.0.0 before the review is finished. I still have open questions about various idea/features etc. But at this point I think they're out of scope for this review and should be treated as typical issues for the |
Ok, let's do this! @machow, Could you please check the final approval in your review so that we know you are finished with this submission? Thanks! |
Done! Sorry for the wait! |
@Batalex has let me know the review will be wrapping up and asked if there's anything else I'd like to add to the codebase first. A this point there's a continuous stream of small to medium sized improvements I'm making, but I don't think any of them need to tie up or be tied up in the review. There is one thing I'd mention though. @Batalex suggested a while ago that See
The feature is merged into |
The previously mentioned |
🎉 We took longer than I expected at first glance, but I think it is for the best. The package was already in a good shape, but it is now definitely more welcoming to new contributors, hence more future-proof. Author Wrap Up TasksThere are a few things left to do to wrap up this submission:
Editor Final ChecksPlease complete the final steps to wrap up this review. Editor, please do the following:
If you have any feedback for us about the review process please feel free to share it here. We are always looking to improve our process and documentation in the peer-review-guide. |
@Batalex Ok, I completed all those items. Setup Zenodo and was able to do a release to get a DOI (added a badge to readme). Also added PyOpenSci badge (very excited about that!). Completed the maintainer survey. @Batalex @machow @isabelizimm Thank you all so much for your help, patience, and time spent on this review. This has been a very valuable learning experience for me and I think |
@Batalex how do I add |
It should be good: https://github.com/pyOpenSci/pyopensci.github.io/blob/main/_data/packages.yml#L110-L150 but the CI job failed: https://github.com/pyOpenSci/pyopensci.github.io/actions/runs/7909228977/job/21589918535. @lwasser I am not familiar with Jekyll, is this a one-off error, or is there something more to it? |
Hello everybody, I see |
oh gosh @Batalex i'm so so sorry i missed your comment above. it looks like this was fixed (i did find some bugs in our ci build a while back and fixed them) ... so hopefully the issue here was fixed!! i sometime miss github pings but am trying better to look at my notification list in a more systematic way. @cmarmo thank you for following up here and closing this!! |
Submitting Author: Justin Gerber(@jagerber48)
All current maintainers: (@jagerber48)
Package Name: Sciform
One-Line Description of Package: A package for converting python numbers (floats, Decimals) into scientific-formatted strings more suitable for reading and presentation.
Repository Link: https://github.com/jagerber48/sciform
Version submitted: 0.24.0
Editor: @Batalex
Reviewer 1: @isabelizimm
Reviewer 2: @machow
Archive:
Version accepted: 0.34.1
JOSS DOI: N/A
Date accepted (month/day/year): 02/07/2024
Code of Conduct & Commitment to Maintain Package
Description
sciform is used to convert python float objects into strings according to a variety of user-selected scientific formatting options including fixed-point and decimal and binary scientific and engineering notations. Where possible, formatting follows documented standards such as those published by BIPM or IEC. sciform provides certain options, such as engineering notation, well-controlled significant figure rounding, and separator customization which are not provided by the python built-in format specification mini-language (FSML) for formatting floats into strings. In addition, sciform provides functionality for formatting pairs of floats as value +/- uncertainty pairs according to a variety of scientific standards.
Scope
Please indicate which category or categories.
Check out our package scope page to learn more about our
scope. (If you are unsure of which category you fit, we suggest you make a pre-submission inquiry):
Domain Specific & Community Partnerships
Community Partnerships
If your package is associated with an
existing community please check below:
For all submissions, explain how the and why the package falls under the categories you indicated above. In your explanation, please address the following points (briefly, 1-2 sentences for each):
Who is the target audience and what are scientific applications of this package?
The target audience for this package includes any scientist trying to print or otherwise display numerical data in a way that is easily readable and conforms to various scientific standards. Numerical data that users may be interested may be numbers (possibly with uncertainties) coming from literature (such as scientific constants), numbers that tabulate raw measurement results or numbers resulting from calculation analyses (such as best-fit algorithms applied to data). Even numbers appearing in plot tick labels can be formatted using sciform. See https://sciform.readthedocs.io/en/stable/examples.html for some example use cases.
Are there other Python packages that accomplish the same thing? If so, how does yours differ?
If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or
@tag
the editor you contacted:Presubmission Inquiry for sciform (float -> string scientific formatting) #114 @NickleDave
Technical checks
For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:
Publication Options
JOSS Checks
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.Note: JOSS accepts our review as theirs. You will NOT need to go through another full review. JOSS will only review your paper.md file. Be sure to link to this pyOpenSci issue when a JOSS issue is opened for your package. Also be sure to tell the JOSS editor that this is a pyOpenSci reviewed package once you reach this step.
Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?
This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.
Confirm each of the following by checking the box.
Please fill out our survey
submission and improve our peer review process. We will also ask our reviewers
and editors to fill this out.
P.S. Have feedback/comments about our review process? Leave a comment here
Editor and Review Templates
The editor template can be found here.
The review template can be found here.
Footnotes
Please fill out a pre-submission inquiry before submitting a data visualization package. ↩
The text was updated successfully, but these errors were encountered: