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

SeaPearl cleanup and bug fix for RL value selection #121

Merged
merged 47 commits into from
May 13, 2021
Merged

Conversation

3rdCore
Copy link
Collaborator

@3rdCore 3rdCore commented May 10, 2021

This is my proposition for SeaPearl cleanup. This pull request goes hand in hand with another pull request on SeaPearlZoo.

Problem : Some part of the source code were obsolete and no longer in the framework of current SeaPearl development. The RL based knapsack example was crashing and returning untrackable errors : ERROR: BoundsError: attempt to access 328-element Vector{Int64} at index [0]

Key features :

  • removed /exemple dir and obsolete examples
  • removed no longer supported Seapearl files: explorer.jl and learned.jl
  • removed dependencies to removed files
  • fixed knapsack_generator.jl, some IntVarView were considered as branchable
  • fixed agent's value / index of value confusion that led to a bug

@3rdCore 3rdCore added bug Something isn't working documentation Improvements or additions to documentation labels May 10, 2021
Copy link
Contributor

@ilancoulon ilancoulon left a comment

Choose a reason for hiding this comment

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

I only wanted to add a few comments to precise some things. But this PR seems to be a great first step forward for SeaPearl. Looking forward to the next ones!

Project.toml Show resolved Hide resolved
# Initialize reward for the next state: not compulsory with DefaultReward, but maybe useful in case the user forgets it
lh.reward.value = 0

# synchronize state: we could delete env.state, we do not need it
# synchronize state: Update the adjacency_matrix of the CPLayerGraph using the CPModel
Copy link
Contributor

Choose a reason for hiding this comment

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

To be precise, the sync_state does not currently do that. This matrix is extracted when calling to_arraybuffer. The only thing that is (currently) synced here is the chosen variable and the possible values (two last columns of the array).

nodevisited[i, j] = model.statistics.numberOfNodes

#TODO understand what this line is doing
if j == 2
set_postfix(iter, Delta=string(nodevisited[i, 1] - nodevisited[i, 2]))
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is to update the progress bar, to show the current difference between the basic heuristic and the learned one (in terms of number of nodes).

test/runtests.jl Show resolved Hide resolved
@3rdCore 3rdCore merged commit 905bc73 into master May 13, 2021
@3rdCore 3rdCore deleted the clean_code branch May 13, 2021 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants