Skip to content

Conversation

@jgeaso1266
Copy link
Collaborator

@jgeaso1266 jgeaso1266 commented Apr 18, 2025

Future Actions

  • update confidence to use linear interpolation

@jgeaso1266 jgeaso1266 requested a review from penguinland April 18, 2025 15:44
Copy link
Contributor

@penguinland penguinland left a comment

Choose a reason for hiding this comment

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

I haven't looked at the "real" code yet, but here's feedback around the edges

Copy link
Contributor

@penguinland penguinland left a comment

Choose a reason for hiding this comment

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

I've got 1 more file to go, but it's the main file.

I worry that some of my feedback is too nitpicky. You're welcome to say "I hear your point, Alan, but I'm not going to do it." These are suggestions, not commands.

Copy link
Contributor

@penguinland penguinland left a comment

Choose a reason for hiding this comment

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

Most of this feedback is minor nitpicks and you can ignore anything you don't like. but the part about what confidence we return is important.

and changing the confidence returned might change the behavior of the tests. I'd just assert that the confidence returned is in the range of values we expect it to be near.

@jgeaso1266 jgeaso1266 requested a review from penguinland April 21, 2025 19:43
@jgeaso1266 jgeaso1266 marked this pull request as ready for review April 21, 2025 19:44
Copy link
Contributor

@penguinland penguinland left a comment

Choose a reason for hiding this comment

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

LGTM! All my remaining feedback is easy to resolve (though some of it is still pretty important).

@jgeaso1266 jgeaso1266 merged commit 54f57b1 into main Apr 22, 2025
1 check passed
@jgeaso1266 jgeaso1266 deleted the RSDK-9981 branch April 22, 2025 16:03

. venv/bin/activate

## using `pyrhon -m PyInastaller` as opposed to `pyinstaller` as the former
Copy link
Contributor

Choose a reason for hiding this comment

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

😆 pyrhon -m PyInastaller should be fixed next PR


if __name__ == "__main__":
asyncio.run(main())
asyncio.run(Module.run_from_registry())
Copy link
Contributor

Choose a reason for hiding this comment

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

Dang, this is way nicer than it used to be! Good idea

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