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

Add 'Random Float' node <3 #4581

Merged
merged 8 commits into from
Sep 26, 2023
Merged

Add 'Random Float' node <3 #4581

merged 8 commits into from
Sep 26, 2023

Conversation

Dekita
Copy link
Contributor

@Dekita Dekita commented Sep 19, 2023

does what it says on the tin :)

What type of PR is this? (check all applicable)

  • Refactor
  • [ x] Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • [x ] Community Node Submission

Have you discussed this change with the InvokeAI team?

  • Yes
  • [ x] No, because, I mentioned it in the discord, but idk who 'officially' noticed it :)

Have you updated all relevant documentation?

  • Yes
  • [ x] No

Description

Adds a random float node to the math category

does what it says on the tin :)
Copy link
Contributor

@Millu Millu left a comment

Choose a reason for hiding this comment

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

Thank you for adding this! A couple of comments:

  • Keeping in line with the random integer node, I don't think we should have a seed input - this would be a used to generate a seed.
  • Also, I know it's fun to have, but do you mind removing the attribution comment to help keep the codebase clean?

altered my random float node as requested by Millu, kept the seeded version as an alternate variant for those that would like to control the randomization seed :)
@Dekita
Copy link
Contributor Author

Dekita commented Sep 21, 2023

just seen this today. I altered the function as requested, but also added a random float with seed function in case folks would like it :)

not sure if i need to do a new pr since its under a new commit?

@Millu
Copy link
Contributor

Millu commented Sep 22, 2023

The commit gets automatically added to the PR!

However, I'm not sure what the value of the node with a "seed" field is. By the nature of the node, a random float will be generated, and the value of surfacing a seed would be to have the same float, which renders the "Random Float" node redundant in the workflow.

I think we should only add the random float without the seed and then go ahead and merge this.

Also, instead of importing random, you can us np.random.uniform(low, high) to keep the code cleaner

@Dekita
Copy link
Contributor Author

Dekita commented Sep 22, 2023

yeah thats fine, i can always just use my seeded float node if i ever need it :D

updated my commit to remove the seeded variant :)

Didnt realize github updated in 'real time' when folks edited their comments etc, thats quite cool. anyway, as requested removed the random import. didnt realise np had it already lol (im a py nub)

@Millu
Copy link
Contributor

Millu commented Sep 22, 2023

Looks good to me!

@psychedelicious
Copy link
Collaborator

psychedelicious commented Sep 26, 2023

After some thought, I think we actually do want our random nodes to have a seed value. If we provide the seed for all RNG (including the random int/float nodes), workflows are fully reproducible.

I've created a feature request for this: #4700

@psychedelicious psychedelicious enabled auto-merge (squash) September 26, 2023 05:53
@psychedelicious psychedelicious merged commit c8b109f into invoke-ai:main Sep 26, 2023
8 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.

None yet

3 participants