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

Master format free space #217

Closed

Conversation

jkonecny12
Copy link
Contributor

We are using magic numbers acquired by testing to reduce the size depending on a used format.

Also add support for Size operators (add,mul...) by float.

Related: rhbz#1224048
Main patch will go to the anaconda later.

# and it's giving us percentage of space left after the format.
# This number is more guess then precise number because this
# value is already unpredictable and can change in the future...
_MagicMetadataMultiplierClass = 1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be named 'Class' if it is a constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah and I think MetadataSizeFactor or something like that would be a better name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I will use MetadataSizeFactor.

@jkonecny12
Copy link
Contributor Author

Yeah sorry my bad.

We are using magic numbers acquired by testing to reduce the size
depending on a used format.

Related: rhbz#1224048
@jkonecny12
Copy link
Contributor Author

I have updated the PR. I hope I did it right :) .

@vpodzime
Copy link
Contributor

vpodzime commented Sep 9, 2015

Looks good to me now.

@vpodzime vpodzime added the ACK label Sep 9, 2015
@jkonecny12
Copy link
Contributor Author

@vpodzime thank you I'll change it locally.

@jkonecny12
Copy link
Contributor Author

Pushed

@jkonecny12 jkonecny12 closed this Sep 10, 2015
@dwlehman
Copy link
Contributor

@jkonecny12 Can you please do a followup that removes the float stuff from blivet.size.Size and instead uses decimal.Decimal for the factors in blivet.formats.fs?

@jkonecny12
Copy link
Contributor Author

Hi @dwlehman, yeah I can do that but why we need that? Isn't it better to do the conversion on one place for every use instead on many places in blivet and anaconda?

@jkonecny12 jkonecny12 reopened this Sep 11, 2015
@jkonecny12
Copy link
Contributor Author

This PR is pushed already but I'm reopening the PR to complete discussion on possible follow-up.

@vpodzime
Copy link
Contributor

@jkonecny12 Can you please do a followup that removes the float stuff from blivet.size.Size and instead uses decimal.Decimal for the factors in blivet.formats.fs?

What would be the benefit of this? I know Decimal intentionally doesn't implement those mathematical operations with floats because they result in a lost of precision, but is Size really the same case? We convert everything to whole bytes anyway so what's the point in enforcing those "infinite precision" rules? Could be I'm missing something here, so let me know. It would be a great help for me for thinking about the GLib implementation of Size.

@dwlehman
Copy link
Contributor

It is about loss of precision from the use of float. We stopped using float for anything related to size some time ago. I forgot that we do round to whole bytes, which means this may be more about convention/principle than practical need.

@vpodzime
Copy link
Contributor

I forgot that we do round to whole bytes, which means this may be more about convention/principle than practical need.

Good to know, I wasn't sure I wasn't missing something here. Let's discuss the approach to this at the meeting.

@jkonecny12
Copy link
Contributor Author

Output from the meeting is this follow-up #229 .
Closing this issue again.

@jkonecny12 jkonecny12 closed this Sep 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants