Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

fix math of calculating scaled price for marketplace #3976

Merged
merged 2 commits into from
Mar 8, 2019

Conversation

etam
Copy link
Contributor

@etam etam commented Mar 8, 2019

  1. revert from msg.price / task.header.max_price back to task.header.max_price / msg.price
  2. put this calculation, with handling of 0, into a function in marketplace module.

Related issues: #3453, #3479, #3964

@etam etam self-assigned this Mar 8, 2019
@ghost ghost added the in progress label Mar 8, 2019
@etam etam force-pushed the fix_free_providers_again branch from 5e16c39 to 6e66144 Compare March 8, 2019 10:52
@etam etam marked this pull request as ready for review March 8, 2019 11:06
@etam etam requested review from Krigpl and kmazurek March 8, 2019 11:06
golem/task/tasksession.py Show resolved Hide resolved
golem/marketplace/offerpool.py Outdated Show resolved Hide resolved
@@ -9,6 +10,12 @@
logger = logging.getLogger(__name__)


def scale_price(task_price: float, offered_price: float) -> float:
if offered_price == 0:
return sys.float_info.max # using float('inf') complicates calculations in rust part
Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of complications are we talking about? It fails with an error? There's test_rust.py which is supposed to test proper integration with Rust and I'd expect a test there that guarantees no "complications in rust part".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The complication was not because of python-rust communication of float('inf') value, but because of calculations done in order_providers_impl (for which I expanded tests in marketplace.py file).
The problem is when alpha = 0, because inf * 0 = nan. And using inf would totally break cases when 0 < alpha < 1, because the score would be always inf.
By using float max it's at least giving a good impression of behaving correctly. I tried to talk with @mbenke about it, but he's not responding right now.

tests/golem/marketplace/test_offerpool.py Outdated Show resolved Hide resolved
@etam etam force-pushed the fix_free_providers_again branch 2 times, most recently from 68a6206 to ed2fea7 Compare March 8, 2019 13:18
tests/golem/marketplace/test_rust.py Outdated Show resolved Hide resolved
@etam etam force-pushed the fix_free_providers_again branch from ed2fea7 to fe62254 Compare March 8, 2019 13:32
@etam etam added the _current label Mar 8, 2019
@etam etam force-pushed the fix_free_providers_again branch from fe62254 to bb471cc Compare March 8, 2019 13:40
@codecov
Copy link

codecov bot commented Mar 8, 2019

Codecov Report

Merging #3976 into b0.19 will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            b0.19    #3976      +/-   ##
==========================================
+ Coverage   89.18%   89.19%   +<.01%     
==========================================
  Files         205      205              
  Lines       18315    18320       +5     
==========================================
+ Hits        16335    16341       +6     
+ Misses       1980     1979       -1

@etam etam force-pushed the fix_free_providers_again branch from bb471cc to 2c32d56 Compare March 8, 2019 13:50
@etam etam merged commit 2e8afeb into b0.19 Mar 8, 2019
@ghost ghost removed the in progress label Mar 8, 2019
@etam etam deleted the fix_free_providers_again branch March 8, 2019 14:14
@etam etam removed the _current label Apr 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants