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

common/lib/camera: Test docs #190

Merged

Conversation

e10harvey
Copy link
Collaborator

@e10harvey e10harvey commented Nov 25, 2024

Purpose

Test and complete docs in common/lib/camera

Summary of changes

Update test_DocStringsExists to include all members of common/lib/camera. Add missing doc strings

Implementation notes

Ran pytest -s -k test_docstrings

Submission checklist

  • Target branch is develop, not main
  • Existing tests are updated or new tests were added
  • opencsp/test/test_DocStringsExist.py are verified to include this change or have been updated accordingly
  • .rst file(s) under doc/ are verified to include this change or have been updated accordingly

Additional information

#189 should be merged before this is reviewed.

@e10harvey e10harvey force-pushed the doc_strings_exist_common_camera branch from 3a108cd to 4669a76 Compare November 25, 2024 20:55
@e10harvey e10harvey mentioned this pull request Nov 25, 2024
4 tasks
@e10harvey e10harvey force-pushed the doc_strings_exist_common_camera branch from 4669a76 to f329198 Compare November 27, 2024 14:17
@e10harvey e10harvey force-pushed the doc_strings_exist_common_camera branch 2 times, most recently from 952f23a to 236f2e6 Compare December 12, 2024 14:19
@braden6521 braden6521 force-pushed the doc_strings_exist_common_camera branch from 236f2e6 to 8ef123b Compare December 17, 2024 18:08
@braden6521
Copy link
Collaborator

Thank you @e10harvey for this code update. Everything technical looks good; thanks for adding documentation where it's needed. I do have one comment relating to UCamera.py:

Currently, UCamera.py is an out-of-date camera class that is part of the UFACET code base. Currently, it is only used in UFACET scripts in the contrib/app/ufacet-s and app/ufacet-s directories. I believe (although I am not the expert on UFACET), that the UFACET code base is somewhat unfinished and not unit tested fully, and in some cases not at all. How to bring the Ufacet code up to OpenCSP standards is a problem for another time. But for now, I propose moving the UCamera class into the app/ufacet-s directory. The motivation behind this is that is reduces clutter in the camera directory and reduces potential confusion when someone new is trying to use OpenCSP for the first time. I believe at one point the thought was to integrate UCamera.py with Camera.py, but that likely won't happen any time soon, so maybe this is the next best thing.

Let me know what you think.

@e10harvey
Copy link
Collaborator Author

Thank you @e10harvey for this code update. Everything technical looks good; thanks for adding documentation where it's needed. I do have one comment relating to UCamera.py:

Currently, UCamera.py is an out-of-date camera class that is part of the UFACET code base. Currently, it is only used in UFACET scripts in the contrib/app/ufacet-s and app/ufacet-s directories. I believe (although I am not the expert on UFACET), that the UFACET code base is somewhat unfinished and not unit tested fully, and in some cases not at all. How to bring the Ufacet code up to OpenCSP standards is a problem for another time. But for now, I propose moving the UCamera class into the app/ufacet-s directory. The motivation behind this is that is reduces clutter in the camera directory and reduces potential confusion when someone new is trying to use OpenCSP for the first time. I believe at one point the thought was to integrate UCamera.py with Camera.py, but that likely won't happen any time soon, so maybe this is the next best thing.

Let me know what you think.

I agree and I filed #196 for this.

@e10harvey e10harvey merged commit 1211314 into sandialabs:develop Dec 19, 2024
4 checks passed
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.

2 participants