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

Additional Dependencies #372

Merged

Conversation

sahiljhawar
Copy link
Member

@sahiljhawar sahiljhawar commented Jul 22, 2024

@atoivonen13 Reqs and test for additional dependencies. Move #369 here

Installation CI only runs when *_requirements.txt or toml file is changed


- name: Install NMMA (base)
run: |
python -m pip install --no-cache-dir 'nmma @ git+https://github.com/sahiljhawar/nmma.git@dependencies'
Copy link
Member Author

Choose a reason for hiding this comment

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

change git link after merge

@sahiljhawar
Copy link
Member Author

@atoivonen13 you can add more files here

@atoivonen13
Copy link
Contributor

@sahiljhawar you want me to move the changes from #369 here?

@sahiljhawar
Copy link
Member Author

@atoivonen13 This PR already contains the changes you made in #369. I mean, if you have more optional dependency files, you can add them in this PR.

@atoivonen13
Copy link
Contributor

@sahiljhawar it does not appear I can push to your fork, but you can add sklearn as its own file and optional dependency. I think the thought was if we were going to make tensorflow optional, we might as well make sklearn option, as users usually use one or the other

@sahiljhawar
Copy link
Member Author

okay. But there should be atleast one ML package which installs with NMMA and anyone can right away start with very basic analysis. What do you think @atoivonen13 ?

@atoivonen13
Copy link
Contributor

@sahiljhawar first off, looks like pyFFTW made a release. Second I was just chatting with Michael about making sklearn a separate file and he thought it makes sense to be agnostic to the methods and just be clear in documentation

sahiljhawar and others added 9 commits July 30, 2024 18:45
linux packages

refact

no need to remove

name change

add tf
ubuntu

dependecy check for macos

macos conditional

hdf5

add ubuntu condition

pip install

try basictex

change cache
@sahiljhawar
Copy link
Member Author

@atoivonen13 yes I know about PyFFTW, already pushed Python 3.12 updates to the repo. also added new file for sklearn

@sahiljhawar
Copy link
Member Author

I guess, need to write a few notes on MacOS installation (and Python 3.12 in particular)

@atoivonen13
Copy link
Contributor

Thanks @sahiljhawar I was mainly just checking in to see when this would be merged and we would do a release. Is there anything else I need to do here?

@sahiljhawar
Copy link
Member Author

@atoivonen13, I think this is done. Waiting for @mcoughlin approval for merge.
Also, do we need make a release right now (0.2.1)?

Copy link
Member

@mcoughlin mcoughlin left a comment

Choose a reason for hiding this comment

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

Sorry! LGTM

@mcoughlin mcoughlin merged commit 77611a0 into nuclear-multimessenger-astronomy:main Jul 31, 2024
17 checks passed
@atoivonen13
Copy link
Contributor

@sahiljhawar A release would be nice when you can so I can make sure everything works with em-bright, but it does not need to be done today or anything. I figured between this and the pyfftw a release is merited, are we waiting on anything else to include in 0.2.1?

@sahiljhawar
Copy link
Member Author

I remember from one of the NMMA calls that Henrik has something to include in the next release, however I am not super sure about it.

@mcoughlin
Copy link
Member

@sahiljhawar Henrik says we can go ahead and will get it into the next one.

@sahiljhawar
Copy link
Member Author

@atoivonen13 @mcoughlin Done!

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.

3 participants