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

Proposed fixes to control.py and utils.py #56

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Proposed fixes to control.py and utils.py #56

wants to merge 5 commits into from

Conversation

spetey
Copy link

@spetey spetey commented Dec 15, 2021

  1. control.py's action_marginals should sum only over the first time step, 2) utils.py's sample(probabilities) throws an error with the argument "squeezed"

…over the first time step, 2) utils.py's sample(probabilities) throws an error with the argument squeezed
@conorheins
Copy link
Collaborator

conorheins commented Dec 15, 2021

thanks for this @spetey -- before I run the CI test-suite, can you please remove the extra files from your commit (those ones committed besides utils.py and control.py)? That way we don't clog up the repo with additional files

@spetey
Copy link
Author

spetey commented Dec 15, 2021 via email

@conorheins
Copy link
Collaborator

Hey @spetey I've pushed another branch for the documentation that also included the fix to control.py already, so we don't need that commit anymore. So all that remains is to figure out the utils.sample() function, that you're proposing a change to. Let me know what the error is and we can figure out whether dropping that .squeeze() is the right way forward.

@alec-tschantz
Copy link
Collaborator

Ugh, I've tried to reset to HEAD~1 and ignore the "egg-info" directory that was auto-added somehow, but then there were conflicts because the upstream repo had changed, and then it stopped ignoring the "egg-info" ... I find git terribly confusing, I tried to fix it as best I could, and opened another pull request.

I've added a gitignore *.egg.info to a PR, so this won't be an issue going forward

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.

3 participants