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

Names for Data/Storage/StorageState #330

Closed
10 of 26 tasks
guimarqu opened this issue May 2, 2020 · 7 comments · Fixed by #667
Closed
10 of 26 tasks

Names for Data/Storage/StorageState #330

guimarqu opened this issue May 2, 2020 · 7 comments · Fixed by #667
Labels
design documentation Need or contain documentation tests
Milestone

Comments

@guimarqu
Copy link
Contributor

guimarqu commented May 2, 2020

I don't understand how you can restore the state of an empty storage ? The state should be empty ? Maybe we should rename AbstractStorageState to AbstractDataState because it's the state of the AbstractData ?

Originally posted by @guimarqu in #323

waiting to get some ideas.


EDIT (2021/03/01), 2nd EDIT (2021/03/18):

Todo:

  • StorageState becomes Record
  • Storage becomes (Storage)Unit
  • change order of arguments in restore_from_record!
  • StorageContainer becomes StorageUnitWrapper
  • move storage into ColunaBase
  • move StorageDict into Model, then delete AbstractData (new method in model interface)
  • StorageDict becomes Storage
  • unit tests on storage "public" methods

Decisions to make :

Improve dev Exp :

  • RecordsVector -> Vector{Record}
  • StorageDict ? -> Storage
  • UnitTypePair = Pair{DataType, DataType} ? -> removed because now bijection between UnitType & RecordType
  • UnitsUsageDict = Dict{Tuple{AbstractModel, UnitTypePair}, UnitAccessMode} ? -> UnitsUsage

Conversations to resolve :

Variables to rename :

  • storage / storage unit wrapper in storages.jl
  • storagedict / storage

Needs documentation (or remove)

  • initialize_storage_units
  • collect_units_to_restore! (why a UnitsUsageDict as first arg ?)
  • store_records(reform)
  • store_records(reform, record_vector)
  • getstorageunit(model, storage_unit_type)
  • record_type
@guimarqu guimarqu added the discussion Further information is requested label May 2, 2020
@rrsadykov
Copy link
Collaborator

We can rename EmptyStorage to FormulationStorage.
Renaming AbstractStorageState to AbstractDataState is ok for me.
I would also be good to rename RuntimeData (ColGenRuntimeData, TreeSearchRuntimeData, etc) to something else, as now there are two datas in the the algorithms (runtime data and reformulation data). This is not good for the code readability.

@guimarqu
Copy link
Contributor Author

It would be nice if we could remove RunTimeData structures in the long run.

@rrsadykov
Copy link
Collaborator

We need this kind of structure to keep the intermediate data which is needed only during the run of the algorithm

@guimarqu
Copy link
Contributor Author

If you look at column generation, the structure is useless. We use it because we didn't think carefuly about the code design.

@rrsadykov
Copy link
Collaborator

I agree that in column generation is not that useful. However, for example, in the tree search algorithm we need to store the tree somewhere.

@guimarqu guimarqu added this to the v0.4 milestone Jun 1, 2020
@guimarqu
Copy link
Contributor Author

guimarqu commented Mar 1, 2021

Todo (cc @rrsadykov) :

Edit : move the list of tasks to do in top comment.

@guimarqu guimarqu added design documentation Need or contain documentation tests and removed discussion Further information is requested labels Mar 1, 2021
@guimarqu guimarqu linked a pull request Mar 18, 2021 that will close this issue
@guimarqu guimarqu removed a link to a pull request Mar 18, 2021
@guimarqu
Copy link
Contributor Author

guimarqu commented Mar 25, 2021

Review

Public

function restore_from_records!(records::RecordsVector, units_to_restore::UnitsUsageDict)
TO.@timeit Coluna._to "Restore/remove records" begin
for (storagecont, recordid) in records
mode = get(
units_to_restore,
(getmodel(storagecont), gettypepair(storagecont)),
READ_ONLY
)
restore_from_record!(storagecont, recordid, mode)
end
end
empty!(records) # vector of records should be emptied
end
function remove_records!(records::RecordsVector)
TO.@timeit Coluna._to "Restore/remove records" begin
for (storagecont, recordid) in records
restore_from_record!(storagecont, recordid, NOT_USED)
end
end
empty!(records) # vector of records should be emptied
end
function copy_records(records::RecordsVector)::RecordsVector
recordscopy = RecordsVector()
for (storagecont, recordid) in records
push!(recordscopy, storagecont => recordid)
increaseparticipation!(storagecont, recordid)
end
return recordscopy
end

To store all storage units of a data, we use functions
"store_records!(::AbstractData)::RecordsVector" or
"copy_records(::RecordsVector)::RecordsVector"
Every stored record should be removed or restored using functions
"restore_from_records!(::RecordsVector,::UnitsUsageDict)"
and "remove_records!(::RecordsVector)"

When you create a record vector with copy or store, you then must destruct records using restore or remove. Otherwise memory leaks.

Dev

  • Create a unit storage <:AbstractUnitStorage
  • Define 0 to n record(s) for this unit storage
  • Define Pair : Type{<:Unit} => Type{<:Record} (if no record : use EmptyRecord)
  • Define constructor for the abstract unit storage :
    For every unit a constructor should be defined which
    takes a model as a parameter. This constructor is
    called when the formulation is completely known so the data
    can be safely computed.
  • Define constructor for the record :
    For each record, a constructor should be defined which
    takes a model and a unit as parameters. This constructor
    is called during storing a unit.
  • Define
    """
    restore_from_record!(model, unit, record_state)
    This method should be defined for every triple (model type, unit type, record type)
    used by an algorithm.
    """
    restore_from_record!(model::AbstractModel, unit::AbstractStorageUnit, state::AbstractRecord) =

@guimarqu guimarqu mentioned this issue Aug 17, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design documentation Need or contain documentation tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants