Priority as number #90

hdavid16 · 2023-07-09T00:44:48Z

Replaces #77

change type on priority from Int to Number. This allows passing a Float as a priority.

Advantages:
This is a more general expression of priority which may make it easier at some point to use rank functions (#75).
This also avoids having to normalize (and sometimes scaling) priorities so that they are integer in queueing systems.

codecov-commenter · 2023-07-09T00:46:29Z

Codecov Report

Merging #90 (fbbc732) into master (c62a6e5) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
+ Coverage   98.17%   98.20%   +0.02%     
==========================================
  Files          10       10              
  Lines         274      278       +4     
==========================================
+ Hits          269      273       +4     
  Misses          5        5

Files Changed	Coverage Δ
src/simulations.jl	`100.00% <ø> (ø)`
src/base.jl	`97.91% <100.00%> (ø)`
src/events.jl	`100.00% <100.00%> (ø)`
src/operators.jl	`100.00% <100.00%> (ø)`
src/processes.jl	`100.00% <100.00%> (ø)`
src/resources/containers.jl	`100.00% <100.00%> (ø)`
src/resources/stores.jl	`95.45% <100.00%> (+0.21%)`	⬆️
src/utils/time.jl	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/resources/containers.jl

hdavid16 · 2023-07-09T03:59:50Z

I'll work on resolving the issues next week.

Krastanov · 2023-07-09T05:48:07Z

src/base.jl

@@ -63,7 +63,7 @@ function remove_callback(cb::Function, ev::AbstractEvent)
  i != 0 && deleteat!(ev.bev.callbacks, i)
 end

-function schedule(ev::AbstractEvent, delay::Number=zero(Float64); priority::Int=0, value::Any=nothing)
+function schedule(ev::AbstractEvent, delay::Number=0; priority::Number=0, value::Any=nothing)


It seems a lot of these type restriction can simply be dropped. Types in function signatures are used only for dispatch, they do not provide performance improvements. And it seems types in kwargs do not even participate in dispatch (see https://discourse.julialang.org/t/force-specialization-on-kwargs/34789/14 ). So this makes types for kwargs (and in particular abstract types) not particularly valuable. In most of these situations (function signatures) you can just delete the ::Int, no need to add ::Number.

TODO: should make order of parameters consistent in both resource types (Containers has it as the first parameter. Stores has it as the second).

hdavid16 · 2023-07-29T04:16:23Z

TODO:

should make order of parameters consistent in both resource types (Containers has priority as the first parameter. Stores has it as the second).

hdavid16 · 2023-07-29T05:53:35Z

@Krastanov, I got it to work without introducing a breaking change. One of the tests on GitHub fails on something with makedocs (nightly). Not sure what that is. Let me know what you think of this PR. Thanks

Krastanov · 2023-07-29T14:14:15Z

I will look into this by the end of the weekend. The documentation tests are unrelated and probably fixed in #93

Krastanov · 2023-08-02T18:31:00Z

This looks good to me. Could you add tests that it works reliably?

E.g.:

adding a process with Int priority followed by adding a processes with Float priority
Float followed by Int
Float followed by Float
Int followed by Int

And maybe something with BigInt and BigFloat. Mainly to make sure there is no older code that makes some implicit assumptions and breaks.

That would also test whether we are missing some convert statements.

Krastanov · 2023-08-02T18:32:51Z

In the meantime I will fix some of the currently hidden / broken tests. The failure on nightly is unrelated and comes from MacroTools. I will try to fix it upstream sometime soon (see FluxML/MacroTools.jl#195 (feel free to jump in if you feel like it :D )

Krastanov · 2023-08-02T18:58:40Z

Merging the other pull request caused conflicts. I think I resolved them all, but please double check my merge commit above (and pull it locally if you need to add something more)

Krastanov · 2023-08-02T19:17:21Z

All of the new documentation failures come from a single newly added test. In particular, I think the constructor for Store has a small break in backward compatibility because previously an implicit convert statement. That Store constructor is the only incompatibility I currently see.

hdavid16 · 2023-08-03T00:37:02Z

The error was because that doctest was using the wrong type for capacity. The capacity in a Store has always been UInt. I have fixed this now.

Krastanov · 2023-08-03T01:25:54Z

src/resources/stores.jl

-  put_queue :: DataStructures.PriorityQueue{Put, StorePutKey{T}}
-  get_queue :: DataStructures.PriorityQueue{Get, StoreGetKey}
-  function Store{T}(env::Environment; capacity=typemax(UInt)) where {T}
-    new(env, UInt(capacity), zero(UInt), Dict{T, UInt}(), zero(UInt), DataStructures.PriorityQueue{Put, StorePutKey{T}}(), DataStructures.PriorityQueue{Get, StoreGetKey}())


The explicit UInt(capacity) cast is removed in the new version. That cast is necessary in order to make capacity=10 work as a keyword argument (which used to be the case until this PR).

SimJulia.jl had the UInt signature in the capacity kwarg. I like the idea of using UInt(capacity) instead of imposing this type on the kwarg, making it possible to use capacity=10 as you suggest. However, we need to still convert to UInt so that a Store cannot have a negative capacity.

src/resources/stores.jl

convert to the resource's priority type

Krastanov · 2023-08-03T01:29:51Z

src/resources/stores.jl

 """
    put!(sto::Store, item::T)

 Put an item into the store. Returns the put event, blocking if the store is full.
 """
-function put!(sto::Store{T}, item::T; priority::Int=0) where T


This does not need to be modified, but just FYI, the ::T in the priority kwarg (and the previously existing ::Int) are unnecessary (if I understand Julia's dispatch rules correctly). Basically, type signatures of keyword arguments are not used for dispatch, so they are frequently superfluous.

They are still useful as a type assert though

we now convert to the appropriate type given the type of the Store

Krastanov · 2023-08-03T01:43:41Z

Wonderful! I will fix the documentation break that was introduced with the merge with the other PR and will merge this as well.

Krastanov · 2023-08-03T02:13:11Z

Sorry to be a killjoy... I was going through the rest of the code because I was extremely surprised that the =Inf default values were not triggering UInt(Inf) conversion errors. It took me a bit to find the source of this -- it is not really in anything you had changed.

Rather now that you have made EvenKey a parameterized type, the heap slot in Simulation is not concrete anymore.

It is here:

mutable struct Simulation <: Environment
  time :: Float64
  heap :: DataStructures.PriorityQueue{BaseEvent, EventKey}

I am a bit on the fence about this. BaseEvent is already abstract, so it is not like this is changing much, but it is pushing things in an unpleasant direction.

What I plan to do is to add a few broken tests as a reminder that this is something that should probably be fixed in the future. I will add the tests, create a new issue to have them recorded, and merge this.

Krastanov · 2023-08-03T02:41:57Z

Actually, I do not know how to do a recursive isconcretetype check anyway... So I can not write a useful broken test. Given that the heap key is already abstract, I am not too worried about changes in performance, but in the future this would be something to improve.

hdavid16 · 2023-08-03T02:54:11Z

You know, now that I think about this. I don't think we should have priorities be Number for EventKey and other scheduling related methods. The motivation behind this PR was to allow the user to specify floats for the priorities used when queueing resources. I don't think a user would typically be setting float priorities for Events. I would be in favor of restoring the Int priorities for Events and just keeping the Number priorities for resources. What do you think, @Krastanov ?

Krastanov · 2023-08-03T03:01:31Z

If you have the bandwidth to do that, I would be very much in favor.

hdavid16 added 7 commits July 8, 2023 20:32

Update base.jl

6d46c77

Update events.jl

59d237d

Update containers.jl

f0303d9

Update stores.jl

d8e1320

Update simulations.jl

ba6f5ea

Update time.jl

2efeae2

Update test_resources_containers.jl

7913399

update max priority to Inf

485d520

Krastanov reviewed Jul 9, 2023

View reviewed changes

src/resources/containers.jl Outdated Show resolved Hide resolved

hdavid16 added 3 commits July 8, 2023 21:03

Update max priority to Inf

a4df1e6

Update max priority to Inf

c19c4f6

Update base.jl

fae28d5

hdavid16 marked this pull request as draft July 9, 2023 01:18

hdavid16 added 8 commits July 8, 2023 21:19

Update base.jl

deb26f2

Update base.jl

b60be22

Update base.jl

4f98ac3

Update base.jl

61f0693

user parametric type

e02cda2

Update stores.jl

c3ba26d

use parametric type

1123ccc

use parametric type

d9e16cc

hdavid16 marked this pull request as ready for review July 9, 2023 01:46

hdavid16 added 2 commits July 8, 2023 21:53

fix struct internal function

aa10136

remove new call in StorePutKey

a292a9c

Krastanov reviewed Jul 9, 2023

View reviewed changes

allow priorities to be Number in Containers and Stores.

baa80b4

TODO: should make order of parameters consistent in both resource types (Containers has it as the first parameter. Stores has it as the second).

make parameter type order consistent btw resources

b21674b

Merge branch 'master' into priority_as_number

3b46b15

Krastanov added 2 commits August 2, 2023 15:02

fixup merge

cac0fc1

fixup merge

32fccd3

add tests for resources with different priorities

cedae0b

Krastanov reviewed Aug 3, 2023

View reviewed changes

src/resources/stores.jl Outdated Show resolved Hide resolved

allow put! and get to pass objects of different priority type

54238ec

convert to the resource's priority type

Krastanov reviewed Aug 3, 2023

View reviewed changes

remove type signatures from kwargs in resources

136aecf

fixup

944135a

fixup

fbbc732

Krastanov merged commit bb72add into master Aug 3, 2023
7 of 8 checks passed

hdavid16 mentioned this pull request Aug 3, 2023

priority for all Events is restored to Int #96

Merged

Krastanov deleted the priority_as_number branch June 19, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Priority as number #90

Priority as number #90

hdavid16 commented Jul 9, 2023

codecov-commenter commented Jul 9, 2023 •

edited

Loading

hdavid16 commented Jul 9, 2023

Krastanov Jul 9, 2023

hdavid16 commented Jul 29, 2023 •

edited

Loading

hdavid16 commented Jul 29, 2023

Krastanov commented Jul 29, 2023

Krastanov commented Aug 2, 2023

Krastanov commented Aug 2, 2023

Krastanov commented Aug 2, 2023

Krastanov commented Aug 2, 2023

hdavid16 commented Aug 3, 2023

Krastanov Aug 3, 2023

hdavid16 Aug 3, 2023

Krastanov Aug 3, 2023

Krastanov Aug 3, 2023

hdavid16 Aug 3, 2023

Krastanov commented Aug 3, 2023

Krastanov commented Aug 3, 2023

Krastanov commented Aug 3, 2023

hdavid16 commented Aug 3, 2023

Krastanov commented Aug 3, 2023

Priority as number #90

Priority as number #90

Conversation

hdavid16 commented Jul 9, 2023

codecov-commenter commented Jul 9, 2023 • edited Loading

Codecov Report

hdavid16 commented Jul 9, 2023

Krastanov Jul 9, 2023

Choose a reason for hiding this comment

hdavid16 commented Jul 29, 2023 • edited Loading

hdavid16 commented Jul 29, 2023

Krastanov commented Jul 29, 2023

Krastanov commented Aug 2, 2023

Krastanov commented Aug 2, 2023

Krastanov commented Aug 2, 2023

Krastanov commented Aug 2, 2023

hdavid16 commented Aug 3, 2023

Krastanov Aug 3, 2023

Choose a reason for hiding this comment

hdavid16 Aug 3, 2023

Choose a reason for hiding this comment

Krastanov Aug 3, 2023

Choose a reason for hiding this comment

Krastanov Aug 3, 2023

Choose a reason for hiding this comment

hdavid16 Aug 3, 2023

Choose a reason for hiding this comment

Krastanov commented Aug 3, 2023

Krastanov commented Aug 3, 2023

Krastanov commented Aug 3, 2023

hdavid16 commented Aug 3, 2023

Krastanov commented Aug 3, 2023

codecov-commenter commented Jul 9, 2023 •

edited

Loading

hdavid16 commented Jul 29, 2023 •

edited

Loading