search

MHKiT-Software / MHKiT-Python

MHKiT-Python provides the marine renewable energy (MRE) community tools for data processing, visualization, quality control, resource assessment, and device performance.
https://mhkit-software.github.io/MHKiT/
BSD 3-Clause "New" or "Revised" License
47 stars 45 forks source link

Update requirements and setup files #137

Closed Matthew-Boyd closed 2 years ago

ssolson commented 2 years ago

Hey Matt can you provide a reason as to why we would not allow users to have the latest version of numpy?

I am in favor of any other solution that prevents this proposed change from entering the codebase as numpy is such a large python package that not supporting the latest version is a problem for our package in terms of use and adoption.

rpauly18 commented 2 years ago

@ssolson I do not think that is a change Matt made. If you look in the repo, that requirement already exists. If I recall, I added that a while back due to an incompatibility with v1.21 and had always intended to revisit when a new version of numpy was released. They are up to v1.22 now, so we may want to try updating this requirement to 'numpy>1.21.0'

ssolson commented 2 years ago

Apologies it looked like that was a change being made from the commit diffs but you are right that had been there previously. Let's remove the version requirement for numpy and check if everything runs okay.

rpauly18 commented 2 years ago

@Matthew-Boyd can you update to "numpy>1.21.0' and see if it passes the tests?

rpauly18 commented 2 years ago

Hi @Matthew-Boyd did you make the requested change on the numpy requirement prior to merging? I do see that you just merged one commit but in the future, either myself or @ssolson will merge PRs after review.

Matthew-Boyd commented 2 years ago

@rpauly18 . I think the issue was I submitted this original pull-request from master when it should have been from a feature branch. Pushing today's commit to fix the MHKiT-Python CI probably carried with it these commits.

Answering your question, no I have not yet made the requested change but will still do so. I wanted to get the MATLAB CI working first so I could test this change for that as well. I did not merge or close this pull request (and wouldn't for my own), unless it happened automatically as a consequence of accidentally pushing the original commits.

I created a new Issue to track this: #138