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

UnitsUsageDict -> UnitsUsage #522

Merged
merged 5 commits into from
Jun 1, 2021
Merged

UnitsUsageDict -> UnitsUsage #522

merged 5 commits into from
Jun 1, 2021

Conversation

guimarqu
Copy link
Contributor

@guimarqu guimarqu commented May 4, 2021

Task of #330

I commented EmptyRecord and EmptyStorage because there are not used at the moment.

@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

❗ No coverage uploaded for pull request base (release-0.4.0@69544da). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head eff55f2 differs from pull request most recent head c6e41a6. Consider uploading reports for the commit c6e41a6 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##             release-0.4.0     #522   +/-   ##
================================================
  Coverage                 ?   84.33%           
================================================
  Files                    ?       47           
  Lines                    ?     4595           
  Branches                 ?        0           
================================================
  Hits                     ?     3875           
  Misses                   ?      720           
  Partials                 ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69544da...c6e41a6. Read the comment docs.

@guimarqu guimarqu changed the title UnitsUsageDict -> UnitsAccess UnitsUsageDict -> UnitsUsage May 4, 2021
@guimarqu guimarqu marked this pull request as ready for review May 4, 2021 10:04
@guimarqu guimarqu requested a review from rrsadykov May 4, 2021 10:04
#(master, BasisUnit) => READ_AND_WRITE) # not yet implemented
)
set_permission!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes sense here only to indicate units which will be restored with read-and-write permission (others will be restored with read-only permission). Thus we may rename units_to_restore to units_to_write, and set_permission!() to set_write_permission!(). The latter would take only two parameters instead of three.

units_to_restore,
getstoragewrapper(master, MasterBranchConstrsUnit),
READ_AND_WRITE
)

#adding the first branching constraints
restore_from_records!(units_to_restore, copy_records(parent.recordids))
Copy link
Collaborator

Choose a reason for hiding this comment

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

restore_from_records!() may also take a vector of pairs (model, unit) instead of the vector of triples (only units which will be restored for writing).

@@ -74,52 +74,36 @@ restore_from_record!(model::AbstractModel, unit::AbstractStorageUnit, record::Ab
))


"""
EmptyRecord
# """
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need EmptyRecord for storage units which are not changed after initialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commented because it is not used in the code nor the tests at the moment

EmptyRecordWrapper
"""
# """
# EmptyRecordWrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same here. EmptyRecordWrapper will be needed for empty records.

@guimarqu
Copy link
Contributor Author

@rrsadykov Ok I understand your comments but I would to discuss about the permissions and this method first:

function restore_from_record!(
storage::StorageUnitWrapper{M,SU,R}, recordid::RecordId, mode::UnitPermission
) where {M,SU,R}

I think we can merge this PR in the midtime.

PS: tests pass

@guimarqu guimarqu merged commit cb9b3fd into release-0.4.0 Jun 1, 2021
@guimarqu guimarqu deleted the rm_unittype branch June 1, 2021 08:58
guimarqu added a commit that referenced this pull request Jun 14, 2021
* dev branch to prepare release of 0.4.0

* Move storage & records in ColunaBase (#507)

* Renaming in storage (#509)

* RecordContainer -> RecordWrapper

* state -> record

* rename lot of things, remove getters of Storage not used by Algorithms

* Storage -> StorageUnitWrapper

* Deletion of `AbstractData`  (#510)

* RecordContainer -> RecordWrapper

* state -> record

* rename lot of things, remove getters of Storage not used by Algorithms

* start removing AbstractData structs

* tests ok

* getunit -> getstorageunit;  StorageDict -> Storage

* fix docstring

* Bijection StorageUnit -> Record (#518)

* Bijection StorageUnit -> Record

* address Ruslan's comments

* Implementation of column generation stages  (#525)

* Implementation of column generation stages (for example, heuristic and exact stage)

* Update after conversation with Guillaume + stabilization correction

* Simplification for ColCutGenConquer

* Some more modifs due to Guillaume comments

* Counting the number of exact calls when testing the pricing stages (#530)

* Add bound callback tests (#532)

* add bound callback tests

* include bound callback in runtests

* fix test

* Apply suggestions from code review

Co-authored-by: Guillaume Marques <[email protected]>

* add comment

* Apply suggestions from code review

Co-authored-by: Guillaume Marques <[email protected]>

Co-authored-by: Guillaume Marques <[email protected]>

* Vector of optimizers in `Formulation` (#534)

* vector of optimizers in formulation

* solver_id -> optimizer_id

* add Manifest

* update Manifest

* remove Manifest because does not work

* changes

* rm files

* address Ruslan's comment

* Update src/MathProg/optimizerwrappers.jl

Co-authored-by: Vitor Nesello <[email protected]>

* add Manifest

* change ci

* remove ci change

* rm Manifest

Co-authored-by: Vitor Nesello <[email protected]>

* UnitsUsageDict -> UnitsUsage (#522)

* UnitsUsageDict -> UnitsAccess

* wip

* improve

* tests ok

* Custom data for variables and constraints (#495)

* draft for support of customer data

* custom data in solution

* custom data for cut callback

* computecoeff

* store custom data of solutions in manager

* add Manifest

* rm Manifest

* Support to custom cuts over custom data assigned to columns with new test

* tests ok

Co-authored-by: Artur Alves Pessoa <[email protected]>

* Prototyping custom model/optimizer (#535)

* Start example

* wip draft

* continue

* add map

* works with caching optimizer

* varids in Env

* wrong result

* multiply costs by -1

* fix scaling

* Apply suggestions from code review

* Update test/interfaces/model.jl

Co-authored-by: Lara Pontes <[email protected]>

* Follow up of "custom data" (#538)

* add AbstractCustomData and set/get inc_val

* fix bugs

* remove duplicate methods

* delete unnecessary prefixes and fix some bugs

* revert some changes and update docstring

* custom information for dw sp (#542)

* docstring for restricted master heuristic (#543)

* docstring for restricted master heuristic

* Update src/Algorithm/conquer.jl

* Refactoring `ObjValues` & `OptimizationState` (#544)

* clean + doc

* move doc from objvalues to optstate; tests of objvalues; update ci

* update

* update branching priorioty deprecated method

* tests ok

* tests ok

* tests ok

* delete duplicated tests

Co-authored-by: Ruslan Sadykov <[email protected]>
Co-authored-by: Artur Pessoa <[email protected]>
Co-authored-by: Lara di Cavalcanti Pontes <[email protected]>
Co-authored-by: Vitor Nesello <[email protected]>
Co-authored-by: Lara Pontes <[email protected]>
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.

2 participants