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

Tem example #612

Merged
merged 54 commits into from
Sep 29, 2023
Merged

Tem example #612

merged 54 commits into from
Sep 29, 2023

Conversation

jesper-friis
Copy link
Collaborator

@jesper-friis jesper-friis commented Aug 22, 2023

Description

Example with OTEAPI and OTELib using TEM data.

This example currently depends on a set of other PRs:

Type of change

  • Bug fix & code cleanup
  • New feature
  • Documentation update
  • Test update

Checklist for the reviewer

This checklist should be used as a help for the reviewer.

  • Is the change limited to one issue?
  • Does this PR close the issue?
  • Is the code easy to read and understand?
  • Do all new feature have an accompanying new test?
  • Has the documentation been updated as necessary?

src/dlite-json.c Outdated Show resolved Hide resolved
src/tests/test_json.c Outdated Show resolved Hide resolved
src/utils/strutils.c Outdated Show resolved Hide resolved
src/utils/strutils.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

I have made some comments.

In particular I miss understanding of the changes to Dlite itself. I would like to re-review once the dependency PRs are approved, so that I can test as well.

Also, why have you added a dockerfile? I did not see any documentation on that and how to use it. (I think it is good to add it, but it wold be nice to document that in the README and not only working in a local env).

jesper-friis and others added 5 commits September 8, 2023 23:52
Co-authored-by: Francesca L. Bleken <48128015+francescalb@users.noreply.github.com>
Added main.py script to incopurate this example in the DLite test system.
Changed workflow.py to a test.
Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

Very nice demo. The demo.py script now works.

However, the some of the additional tests are not working. This does not show up here on github because the TEM-example is skipped in ctest. This is probably because the additional requirements are not included in the requirments_dev. Make sure to include the versions as well. Then it should not be necessary to have a requirements.txt inside the tem-demo. If you want to keep it, please add the versions.

So, before merging, please make sure that

  1. TEM-demo is run as part of the ctest.
  2. Tests are passing.
  3. Tests that are not specific for the TEM-demo are moved out into a separate issue that can be resolved independently.
  4. Versions are included in requirements.

@jesper-friis
Copy link
Collaborator Author

I have made some comments.

In particular I miss understanding of the changes to Dlite itself. I would like to re-review once the dependency PRs are approved, so that I can test as well.

Also, why have you added a dockerfile? I did not see any documentation on that and how to use it. (I think it is good to add it, but it wold be nice to document that in the README and not only working in a local env).

Removed the docker-compose file for now, since it is not currently used.

@jesper-friis
Copy link
Collaborator Author

jesper-friis commented Sep 29, 2023

Very nice demo. The demo.py script now works.

However, the some of the additional tests are not working. This does not show up here on github because the TEM-example is skipped in ctest. This is probably because the additional requirements are not included in the requirments_dev. Make sure to include the versions as well. Then it should not be necessary to have a requirements.txt inside the tem-demo. If you want to keep it, please add the versions.

So, before merging, please make sure that

  1. TEM-demo is run as part of the ctest.
  2. Tests are passing.
  3. Tests that are not specific for the TEM-demo are moved out into a separate issue that can be resolved independently.
  4. Versions are included in requirements.

Response:

  1. Addressed in issue Add test for TEM example #655. This is much easier to implement when this PR has been accepted and released.
  2. The tests passes locally for me.
  3. Will be done in issue Add test for TEM example #655.
  4. Done

@jesper-friis jesper-friis merged commit 5c536da into master Sep 29, 2023
@jesper-friis jesper-friis deleted the tem_example branch September 29, 2023 18:09
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