MHKiT-Software / MHKiT-MATLAB

MHKiT-MATLAB 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
15 stars 23 forks source link

V0.3.0 MHKiT #33

Closed rpauly18 closed 3 years ago

rpauly18 commented 3 years ago

This PR contains the new functions and reorganizations for MHKiT v0.3.0.

Still to-do: Tests for all new functions

rpauly18 commented 3 years ago

@kmruehl and @ssolson I still need to develop the tests for these new functions, but they are ready to be reviewed, particularly for the DocString information. Thanks!

ssolson commented 3 years ago

I think you have the loads functions at both the loads level and the sub folder level: image

ssolson commented 3 years ago

It looks like there is still a test data folder. Is your plan to move all of this to example data when you write the tests?

image

rpauly18 commented 3 years ago

Yes, I am working on that as we speak.

kmruehl commented 3 years ago

@rpauly18 do you mean that you would like me to check the API documentation?

rpauly18 commented 3 years ago

@kmruehl yes, or at least do a general proof read. If you have time, it would also be good for you to run some of the functions and/or the example notebooks. Thanks!

ssolson commented 3 years ago

Rebecca to be consistient with python the wave.io folder should have a subfolder called NDBC and then each function would be request data etc. Is this possible/ does it make sense in MATLAB? image

Also will you be able to wrap the WEC-SIM IO? I don't expect it to change in structure at this point.

rpauly18 commented 3 years ago

@ssolson I will not be wrapping the WEC-Sim IO because WEC-Sim data is already in a MATLAB friendly format, so wrapping the Python function would not be necessary/ counter productive.

I could create the sub folder for NDBC, but at this point it would be the only folder in IO, so it might not be necessary.

rpauly18 commented 3 years ago

That being said, it would be useful to develop a WEC-Sim example notebook.

ssolson commented 3 years ago

Of course that makes sense to not wrap it! Agreed on the notebook/ testing the WEC-SIM Matlab data to MHKiT functions.

I would be in favor of creating the NDBC subfolder as we plan to at least add Aiden's work in the future and then the NDBC functionality would remain consisteient e.g. no rework of the examples/ tests and documentation. But its your call.

ssolson commented 3 years ago

Rebecca I am getting this error below when I try to run the contour notebook example: image

I would change the title of section 3 to something like "Find the 100 year contour line " (this should eventually be changed on the Python side too)

line 18 on the definition of period needs semicolon to close the line.

rpauly18 commented 3 years ago

Sterling, what version of Matlab do you have? I do not get that error. @kmruehl can you try running the example ad see if you also get the error?

ssolson commented 3 years ago

'9.8.0.1417392 (R2020a) Update 4' Looks like Update 5 is out so I will install that to see if it fixes the issue.

rpauly18 commented 3 years ago

OK, I really hope MATLAB did not do away with converting numpy to double in 2020, because that will mean just about every wrapper in the package will need to be changed. I am still on 2019.

kmruehl commented 3 years ago

@rpauly18 I can take a look. I have both 2020a and 2018b on my laptop.

rpauly18 commented 3 years ago

@ssolson Have you been able to run any of the other examples? Many of the MHKiT-MATLAB functions make that conversion, so it would be good to see if you get the same error in other notebooks.

ssolson commented 3 years ago

I'm trying to run but I think your feature branch is missing the other data files? e.g. only the wave data is available.

image

Also Update 5 did not fix the issue

ssolson commented 3 years ago

Yes I am getting the error in wave: image

rpauly18 commented 3 years ago

@ssolson I made some updates to how numpy arrays are handled in NDBC_request_data. Can try re-running the environmental contours example to see if this methodology will work for you?

ssolson commented 3 years ago

Hey Rebecca, I installed the 3.1 toolbox and I am getting the same conversion to double error

rpauly18 commented 3 years ago

Darn. OK, I am going to wait until Kelley reports back to proceed. I will need to go through NREL IT to get a copy of R2020, so I cannot test it yet myself.

kmruehl commented 3 years ago

That being said, it would be useful to develop a WEC-Sim example notebook.

I already have one I can add, I developed a *.pyand *.m example at the same time.

rpauly18 commented 3 years ago

Thats great. Thanks, Kelley!

kmruehl commented 3 years ago

@rpauly18 and @ssolson I just ran environmental_contours_example.mlx in 2020a without issue. I'll test the other examples too.

ssolson commented 3 years ago

What python and numpy version do you guys run? I python 3.8.3 and numpy 1.19.1

kmruehl commented 3 years ago

Name: numpy Version: 1.18.1

Python 3.7.6

rpauly18 commented 3 years ago

numpy 1.18.5

Python 3.7.5

I thought Python 3.8 might be an issue, but according to this https://www.mathworks.com/help/matlab/matlab_external/system-requirements-for-matlab-engine-for-python.html it should be fine.

rpauly18 commented 3 years ago

@ssolson Turns out the issue is Python 3.8. I updated and got the same error. I have now reverted back to 3.7. We will need to think about how to handle this issue.

ssolson commented 3 years ago

Super happy we diagnosed the issue. Now I can change the MATLAB instructions to state that it is only compatible with 3.6 and 3.7 as it did before.

Can you switch back to 3.8 and see if you get the WEC SIM io error I was getting on the detrend call?

rpauly18 commented 3 years ago

@ssolson is there any way you can update to Matlab 2020b? According to Matlab forums, 2020a does not support Python3.8. The first Matlab version to support 3.8 is 2020b.

ssolson commented 3 years ago

Yes I am downloading it now. I'll let you know if it fixes the issue.

kmruehl commented 3 years ago

@rpauly18 I've reviewed all of the MATLAB Live examples and made some minor revisions to the examples with https://github.com/MHKiT-Software/MHKiT-MATLAB/pull/33/commits/aacbfbe3c138b362378b9bf6b769e29c4a55cda6, namely:

ssolson commented 3 years ago

Updating to 2020b resolved the double conversion issue for Python 3.8

rpauly18 commented 3 years ago

@ssolson @kmruehl I think this PR is ready for a final review, and hopefully merging.

rpauly18 commented 3 years ago

I am going to wait until we tag the Python release on pip before merging, just in case someone tries to download this before they can pip install the corresponding MHKiT-Python.