Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Adding PO.DAAC data source along with the tests #383

Merged
merged 15 commits into from
Aug 27, 2016
Merged

Adding PO.DAAC data source along with the tests #383

merged 15 commits into from
Aug 27, 2016

Conversation

Omkar20895
Copy link
Contributor

  • This PR adds PO.DAAC data source to ocw.
  • This PR also addresses the required tests.

Any suggestions are welcome, Thanks.

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@agoodm
Copy link
Member

agoodm commented Aug 8, 2016

@Omkar20895 @lewismc Do you know how we will handle this as a dependency? If we opt to not ship the source code from podaac_data_source with OCW, then we will need to add a new conda recipe. We can easily handle this if the code is on PyPi or Github.

We should also consider adding the podaac loader to the list of default loaders in dataset_loader.py once that's sorted out. You will need to update the comments in the class constructor as well as add a reference to the loader function in the _source_loaders dict.

@lewismc
Copy link
Member

lewismc commented Aug 9, 2016

@agoodm we are pushing the third party library to Pypi. It won't be an issue. This PR is just a start and is not ready yet by any means. Thanks fro reviewing.

@lewismc
Copy link
Member

lewismc commented Aug 10, 2016

@Omkar20895 can you make sure that all of this code is PEP8 formatted? Thanks

@lewismc
Copy link
Member

lewismc commented Aug 10, 2016

@Omkar20895 can you also please squash your commits into one and edit the commit message to be "CLIMATE-769 Create data source input for NASA JPL PO.DAAC"

@Omkar20895
Copy link
Contributor Author

@lewismc sure thing!

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@lewismc
Copy link
Member

lewismc commented Aug 11, 2016

@Omkar20895 I'll repeat what I said above

@Omkar20895 can you also please squash your commits into one and edit the commit message to be "CLIMATE-769 Create data source input for NASA JPL PO.DAAC"

Thanks

@lewismc
Copy link
Member

lewismc commented Aug 11, 2016

@jarifibrahim what does this failure mean?
https://travis-ci.org/apache/climate/builds/151333565

@jarifibrahim
Copy link
Member

@lewismc The exception can be fixed by renaming time_range to temporal_boundaries.
But there are many more exceptions in the unit tests. A lot of new code has been added lately but the tests have not been updated. The tests were failing and still the code was merged. I'm working on fixing these tests. Please do not merge until the tests are fixed.

@Omkar20895
Copy link
Contributor Author

@lewismc I got that earlier, i had some merge conflicts while I was squashing and it was late at night, so I figured I would solve them in the morning, I will work on it now, sorry for the delay, Thanks.

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@lewismc
Copy link
Member

lewismc commented Aug 11, 2016

@jarifibrahim thanks for looking into this.
@Omkar20895 please make sure that the tests are updated as is documentation along with your pull requests. We will keep reviewing this until it is stable and is well tested and the documentation reflects the newly added modules.
Please address all of the above.

@huikyole
Copy link
Contributor

@Omkar20895 @lewismc Would it be easier if we merge this PR anyway and updating the tests later?

@Omkar20895
Copy link
Contributor Author

@lewismc I think the failure of the tests is due to some other module but not podaac, I have run the tests cases in my local machine all the tests related to podaac data source are running OK. Thanks.

@huikyole I agree with you, the branch is quite stable with just one documentation update on its way. With that update I think we can merge this branch. Thanks.

@Omkar20895
Copy link
Contributor Author

Omkar20895 commented Aug 12, 2016

The documentation of dap.py is broken and also when I try to build the documentation locally I get the error, theme not found. Someone please check this. Thanks.

Edit : I have changed the documentation theme to the theme that we used in podaacpy toolkit and it worked, so am I allowed to change the theme here?? Thanks.

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@Omkar20895
Copy link
Contributor Author

Omkar20895 commented Aug 12, 2016

In this latest commit I have added,

  • An example on usage of podaac data source which evaluates Temporal STD metric, the example on running generates a contour map and I have cross checked it to the map generated in panoply.
  • A documentation update to podaac data source.

Issues to be addressed

  • Outdated theme in the documentation.
  • Broken documentation in dap.py, the

_convert_times_to_datetime() function is dropped by automodule of sphinx and hence does not appear in the documentation. This is due to the underscore in the beginning of the funciton name.

Thanks.
@lewismc @huikyole @jarifibrahim

@lewismc
Copy link
Member

lewismc commented Aug 12, 2016

@huikyole if the tests are broken then we are not merging... that doesn't make sense.
@Omkar20895
Please log issues for both

Outdated theme in the documentation.

and

Broken documentation in dap.py,

Please submit pull requests in addition to this PR such that the tests become stable in TravisCI.

Once that is done, please open Tickets for both of the above issues you;ve highlighted and I will work with you to resolve them.
Thanks @Omkar20895 good work.

d = netcdf_dataset(path, mode='r')
dataset = d.variables[variable]

# By convention, but not by standard, if the dimensions exist, they will be in the order:
Copy link
Member

Choose a reason for hiding this comment

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

Make this Python function documentation rather than hidden docs please.

@lewismc
Copy link
Member

lewismc commented Aug 25, 2016

@Omkar20895 here you go
https://pypi.python.org/pypi/podaacpy
Updated version is 1.0.2

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 80.622% when pulling 0a9d6b4 on Omkar20895:CLIMATE-769 into 91ec7f3 on apache:master.

@@ -9,3 +9,4 @@ esgf-pyclient>=0.1.6
python-dateutil>=2.5.3
mock>=2.0.0
myproxyclient>=1.4.3
podaacpy>=1.0.1
Copy link
Member

Choose a reason for hiding this comment

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

Once this is upgraded to 1.0.2 we are good to merge this patch. Thanks @Omkar20895

@Omkar20895
Copy link
Contributor Author

Finally!! The build passed OK! 😄 @lewismc I will add in a commit and merge it, Thanks.

@lewismc
Copy link
Member

lewismc commented Aug 26, 2016

+1 please do.
BTW @Omkar20895 did you check out https://github.com/lewismc/podaacpy/issues/64
LOL
We should still commit and improve the podaac_datasource based upon what does and does not work within the PO.DAAC WS API

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@Omkar20895
Copy link
Contributor Author

@lewismc I think the new version luckily does not affect the present behaviour of podaac_datasource since podaac_datasource depends solely on search_granule and dap which are intact in the new version compared to the previous version. Thanks.

@lewismc
Copy link
Member

lewismc commented Aug 26, 2016

Cool, if travis-ci build is stable then please merge with master.

@Omkar20895
Copy link
Contributor Author

Omkar20895 commented Aug 26, 2016

That's odd, the tests failed in dap.py and I haven't even touched dap.py in this PR.
@lewismc @jarifibrahim

edit : I am sending a trivial update to trigger travis-ci again to check if its a false alarm raised by travis. Thanks.

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@Omkar20895
Copy link
Contributor Author

@lewismc you are right, the search_granule has been compromised 😅 , they have removed the shortName option from it, this PR is gonna take a while to get merged now. we need to update podaacpy and release a new version.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 80.622% when pulling 7d0daf0 on Omkar20895:CLIMATE-769 into 91ec7f3 on apache:master.

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@OCWJenkins
Copy link

Merged build triggered. Test Failed.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 80.617% when pulling 39960b5 on Omkar20895:CLIMATE-769 into 91ec7f3 on apache:master.

@Omkar20895
Copy link
Contributor Author

To conclude the PR:

  • I have integrated PO.DAAC data source to OCW.
  • All tests run 'OK' and tests can be added further.
  • I have also added an example for using podaac.
  • The data source is upto date to the changes occured recently in PO.DAAC WS, which affected the data source to be integrated into OCW at a very minute level and these differences have been addressed.

@lewismc can I get a '+1' for merge now? Thanks.

@lewismc
Copy link
Member

lewismc commented Aug 26, 2016

Hi @Omkar20895 yes please. Go ahead and merge. We can work on Podaacpy 1.1.0 and then update here when necessary. Thanks very much. Excellent work and very well done over the summer.

@asfgit asfgit merged commit 39960b5 into apache:master Aug 27, 2016
@Omkar20895 Omkar20895 deleted the CLIMATE-769 branch August 27, 2016 03:43
@Omkar20895
Copy link
Contributor Author

Pheww! The PR is merged and with this my GSOC would have come to an end, but obviously PO.DAAC JPL doesn't want that(:P). Anyways it has been a really good learning experience and I would definitely continue to contribute to this project.
@lewismc Thanks for being such a good mentor.

P.S : @agoodm we need to starting working on making a conda recipe for podaacpy. Thanks.

@agoodm
Copy link
Member

agoodm commented Aug 27, 2016

@Omkar20895 I have already made the JIRA issue for it. I can quickly get this done next week.

@Omkar20895
Copy link
Contributor Author

@agoodm thank you very much!

@lewismc
Copy link
Member

lewismc commented Aug 27, 2016

@agoodm please reach out to us for any issues.
@Omkar20895 excellent work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants