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

feat: Package as derivative-calculator library #1

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

matthewfeickert
Copy link

👋 @nikosarcevic Here's a drive-by PR that packages things up. My feelings won't be hurt at all if you didn't have any interest in this and aren't interested at the moment! :)

  • Package up as derivative-calculator library using hatch and dynamically version with hatch-vcs.
    • Expose derivative_calculator.DerivativeCalculator API on import of derivative_calculator.
import derivative_calculator
derivative_calculator.DerivativeCalculator  # <class 'derivative_calculator.derivative_calculator.DerivativeCalculator'>
  • Add GitHub's standard Python project .gitignore.
  • Add barebones GitHub Actions based CI that at the moment just verifies that the package can be installed.
  • Update README to reflect that the tool can be installed with pip.

* Use hatch and hatchling to build and install the library as a package
  and then use hatch-vcs to version it dynamically through Git and Git
  tags.
* seaborn and scipy are never used, so remove the imports.
* Use the standard Python project .gitignore from GitHub and then also
  ignore the dynamically generated version file.
pyproject.toml Outdated
Comment on lines 15 to 16
authors = [ {name = "Nikolina Sarcevic", email = "n.sarcevic2@newcastle.ac.uk"} ]
maintainers = [ {name = "Nikolina Sarcevic", email = "n.sarcevic2@newcastle.ac.uk"} ]
Copy link
Author

Choose a reason for hiding this comment

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

I used university emails here, but maybe you want something else.

Copy link
Owner

Choose a reason for hiding this comment

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

yeah I was debating this but I prefer gmail as it is permanent. Newcastle will be gone next year so better to keep gmail

Comment on lines +38 to +39
"seaborn>=0.12.0", # requires matplotlib, scipy, numpy
"numdifftools>=0.9.40",
Copy link
Author

Choose a reason for hiding this comment

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

These lower bounds aren't tested, just eyeballed as "reasonable" assumptions.

Comment on lines -4 to -5
import scipy as sp
import seaborn as sns
Copy link
Author

Choose a reason for hiding this comment

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

scipy and seaborn are never actually used, so the imports are removed.

Copy link
Owner

Choose a reason for hiding this comment

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

ah good catch! those are my leftovers forgot to remove them

Comment on lines +69 to +71
```
python -m pip install .
```
Copy link
Author

Choose a reason for hiding this comment

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

Could also show that this would allow for install directly from the GitHub url, but I'll leave that up to you if you want that or not.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes! much obliged!

@nikosarcevic
Copy link
Owner

👋 @nikosarcevic Here's a drive-by PR that packages things up. My feelings won't be hurt at all if you didn't have any interest in this and aren't interested at the moment! :)

  • Package up as derivative-calculator library using hatch and dynamically version with hatch-vcs.

    • Expose derivative_calculator.DerivativeCalculator API on import of derivative_calculator.
import derivative_calculator
derivative_calculator.DerivativeCalculator  # <class 'derivative_calculator.derivative_calculator.DerivativeCalculator'>
  • Add GitHub's standard Python project .gitignore.
  • Add barebones GitHub Actions based CI that at the moment just verifies that the package can be installed.
  • Update README to reflect that the tool can be installed with pip.

oh this is grand.
I was thinking if it would be necessary to first see if it can be:

  • optimized
  • made more general. so far is very simplistic and some people might have to modify the code in order to deal with different data structures. for example my derivatives for one parameter will be a matrix as my data vector is an angular power spectrum Cl. so my data vector is Cls X ells.
  • add some unit tests?
    • anything else?
  • what do you think @matthewfeickert

* Add CI that simply installs the package.
  More tests can be added in the future.
@matthewfeickert
Copy link
Author

matthewfeickert commented Dec 14, 2023

I was thinking if it would be necessary to first see if it can be:

  • optimized
  • made more general. so far is very simplistic and some people might have to modify the code in order to deal with different data structures. for example my derivatives for one parameter will be a matrix as my data vector is an angular power spectrum Cl. so my data vector is Cls X ells.
  • add some unit tests?

All of this is easier to do IMO if you start with a package and then move forward. You, and anyone else, can hack at it all you want but still keep the package format by installing in editable mode

python -m pip install -e .

where changes to the source files under src/ will be reflected as you make them (due to pip doing fancy symlinking behind the scenes).

Testing should be added, but testing with pytest as a package is way easier to make sure you're doing what you think you're doing then a collection of scritps.

@nikosarcevic
Copy link
Owner

I was thinking if it would be necessary to first see if it can be:

  • optimized
  • made more general. so far is very simplistic and some people might have to modify the code in order to deal with different data structures. for example my derivatives for one parameter will be a matrix as my data vector is an angular power spectrum Cl. so my data vector is Cls X ells.
  • add some unit tests?

All of this is easier to do IMO if you start with a package and then move forward. You, and anyone else, can hack at it all you want but still keep the package format by installing in editable mode

python -m pip install -e .

where changes to the source files under src/ will be reflected as you make them (due to pip doing fancy symlinking behind the scenes).

Testing should be added, but testing with pytest as a package is way easier to make sure you're doing what you think you're doing then a collection of scritps.

ok so -- I never did this before! do you have some time to chat? like quick zoom? also I would call the package differently as it is not my method but it is my implementation. lmk if u are free for like 20 min

Comment on lines +11 to +13
schedule:
- cron: '23 1 * * 0'
workflow_dispatch:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to run this weekly? Wouldn't it make more sense to run the CI on a branch before it is merged?

Copy link
Author

@matthewfeickert matthewfeickert Dec 15, 2023

Choose a reason for hiding this comment

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

Yes, and it does.

The on block runs:

  • On push events to main.
  • On pull_request events that target main.
  • On schedule events with a frequency of weekly.
  • On demand via workflow_dispatch events.

As for "why run CI on a cron basis" the short answer is that as the dependencies are not static (this is currently setup like a library and not an application that would be dpeloyed in an environment with a lock file) you want to test that things work with the latest dependencies even if you havent' changed the code. In this PR there are no tests yet, but if there was this would apply.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right. I'm not very familiar with GitHub workflows, so I missed that there are several triggers for it.

I hadn't considered the changing dependency angle, but am still not sure I understand the value of it. At least in this case, the dependencies are basically standard libraries of python. Because of this, they have their own extended testing suite to ensure that results remain the same. Is it reasonable to expect that tests here will catch something that the tests there will miss?

pyproject.toml Outdated Show resolved Hide resolved
@matthewfeickert
Copy link
Author

also I would call the package differently as it is not my method but it is my implementation. lmk if u are free for like 20 min

Probably not this week as I'm traveling at the moment and have nearly dawn to dusk meetings on Friday. 😬 Whatever naming you'd like is totally fine. 👍 Just suggest it in a PR reveiew comment.

@nikosarcevic
Copy link
Owner

also I would call the package differently as it is not my method but it is my implementation. lmk if u are free for like 20 min

Probably not this week as I'm traveling at the moment and have nearly dawn to dusk meetings on Friday. 😬 Whatever naming you'd like is totally fine. 👍 Just suggest it in a PR reveiew comment.

grand. no worries, we can do this on gh. I will be reviewing it over the next few days as I need to do something else urgently. thanks so much for this, it is great!!

@lonbar
Copy link
Collaborator

lonbar commented Dec 15, 2023

@matthewfeickert do you maybe have a nice reference for writing python packages (basically something that goes over most of what you did in this PR)? I've never tried to make a package pip installable, and although there is a lot of information out there I find it a bit difficult to get a comprehensive overview.

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

3 participants