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

Update test_cli.py #242

Closed
wants to merge 10 commits into from
Closed

Conversation

emprzy
Copy link
Collaborator

@emprzy emprzy commented Jun 25, 2024

I am curious if this is all that I needed to do. I changed some other things (inlcuding adding a clause in ci.yml) that I thought would be necessary, but upon second thought I deleted them because I suspected them to be unnecessary.
Now, the only change here is that I added the function we talked about to test_cli.py.
Please let me know what changes need to be made, if any.

Added a function to test inference_main.R
@emprzy emprzy requested a review from jcblemai June 25, 2024 17:12
@emprzy
Copy link
Collaborator Author

emprzy commented Jun 25, 2024

I changed a typo in the code. Still failing. Will take another look later

Attempting to fix error
@jcblemai
Copy link
Collaborator

Yes @emprzy , this is enough (no need to change the ci.yml). The test fails because inference_main calls inference_slot on the wrong path, because FLEPI_PATH is not set. Add to your command line: --flepi_path ../../ to tell it that the root of the flepiMoP repo is two directory backwards

emprzy and others added 5 commits June 28, 2024 16:04
including `--FLEPI_PATH` so that inference_main can find inference_slot
-- flepi_path ../../ addition to the end of the CL command
@jcblemai
Copy link
Collaborator

The tests are failing because of #197

@emprzy emprzy closed this Aug 26, 2024
@jcblemai
Copy link
Collaborator

jcblemai commented Oct 2, 2024

@emprzy did this get merged or not ? this seems very useful ?

@jcblemai jcblemai reopened this Oct 2, 2024
@emprzy
Copy link
Collaborator Author

emprzy commented Oct 2, 2024

@emprzy did this get merged or not ? this seems very useful ?

Ah, I forgot about this PR. This is totally my fault because I did a horrible job properly documenting within the actual PR, but do you know what this was intended to address?

@jcblemai
Copy link
Collaborator

jcblemai commented Oct 4, 2024

This was adding a test of the R inference as part of the automatic test suite, I can look in ways to finish it, I think it was failing because it was blocked by another issue which is now resolved. Perhaps it even works now without doing anything. Let's see.

@pearsonca
Copy link
Contributor

I recommend close this PR, but don't chuck the branch.

Once we've dealt with #336, I'm planning to integrate inference into the CLI, including support to use the R scripts. The skeleton here will be useful, but the core invocation is going to be different.

@TimothyWillard
Copy link
Contributor

@emprzy @pearsonca I think this PR has fallen quite a way behind main at this point. I think we might be better off closing this off and re-addressing from the original issue. What is the GitHub issue for this PR (if there is one)?

@pearsonca
Copy link
Contributor

@emprzy @pearsonca I think this PR has fallen quite a way behind main at this point. I think we might be better off closing this off and re-addressing from the original issue. What is the GitHub issue for this PR (if there is one)?

I'll grab the inference test into my work on flepimop calibrate ..., so this can be closed as OBE. Should be under #378 now.

@emprzy
Copy link
Collaborator Author

emprzy commented Nov 18, 2024

@emprzy @pearsonca I think this PR has fallen quite a way behind main at this point. I think we might be better off closing this off and re-addressing from the original issue. What is the GitHub issue for this PR (if there is one)?

I'm very much in favor of closing it. I think perhaps we should check with @jcblemai though?

@TimothyWillard
Copy link
Contributor

TimothyWillard commented Nov 18, 2024

@jcblemai is not going to be as involved day-to-day, I think we are safe closing this without waiting.

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.

4 participants