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

Core rewrite #21

Open
wants to merge 54 commits into
base: main
Choose a base branch
from
Open

Core rewrite #21

wants to merge 54 commits into from

Conversation

KubEF
Copy link
Contributor

@KubEF KubEF commented Sep 13, 2024

Some fixes and tonnes of rewriting from here.
I have some main questions in here

  • Did I split the module wisely?
  • Did I break the idea?
  • How can I test it?

@KubEF KubEF force-pushed the core-rewrite branch 2 times, most recently from 0543f05 to 4f1e47f Compare September 13, 2024 17:48
@KubEF KubEF requested review from WoWaster and gsvgit September 13, 2024 17:49
lamagraph-core/lamagraph-core.cabal Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Node.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Node.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Node.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Node.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Node.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Node.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Node.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Node.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Outdated Show resolved Hide resolved
Copy link
Member

@WoWaster WoWaster left a comment

Choose a reason for hiding this comment

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

This is a first look at this code. It's very complicated both syntactically and semantically. I couldn't wrap my head about semantics of this code today, just updateMM requires ton of courage to approach. Thus, here mostly syntactical comments, but I'll definitely give this code another try in some time.

lamagraph-core/src/Core/MemoryManager.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/Reducer.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/MemoryManager.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/MemoryManager.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/MemoryManager.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/MemoryManager.hs Outdated Show resolved Hide resolved
KubEF added 10 commits November 15, 2024 15:52
Simplified update function
Add simpliest doctest
Add type alias for Ram
Move most of helping function to top-level
Replace ^. to view
Add epsilon agent
Move concrete reduction rules into separate functions
add docs with images to them
Copy link
Member

@gsvgit gsvgit left a comment

Choose a reason for hiding this comment

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

Yet another part of comments.

ReduceRuleResult 0 2 2
lNode |><| rNode = case (lNode ^. nodeType, rNode ^. nodeType) of
(Apply, Abs) -> applyToLambdaRule lNode rNode
(Abs, Apply) -> applyToLambdaRule lNode rNode
Copy link
Member

Choose a reason for hiding this comment

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

Should it be applyToLambdaRule rNode lNode?

data AgentSimpleLambda
= Apply
| Abs
| Eps
Copy link
Member

Choose a reason for hiding this comment

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

erase in Lafont
delete agent in Interaction nets in Ocaml


data AgentSimpleLambda
= Apply
| Abs
Copy link
Member

Choose a reason for hiding this comment

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

Full name.

ActivePair portsNumber ->
ReduceRuleResult nodesNumber edgesNumber portsNumber ->
Delta nodesNumber edgesNumber portsNumber
toDelta acPair reduceResult = Delta (reduceResult ^. nodes) (reduceResult ^. edges) acPair
Copy link
Member

Choose a reason for hiding this comment

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

acPair -> activePair
reduceResult -> reductionResult

Copy link
Contributor Author

Choose a reason for hiding this comment

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

activePair clash with _activePair in Delta. So, because of lenses it is function name. acPair is necessary compromise.

(Node portsNumber -> Node portsNumber -> ReduceRuleResult nodesNumber edgesNumber portsNumber) ->
Signal dom (ActivePair portsNumber) ->
Signal dom (Delta nodesNumber edgesNumber portsNumber)
reducer transFunction activeP = toDelta <$> activeP <*> reduceRuleRes
Copy link
Member

Choose a reason for hiding this comment

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

activeP -> activePair
transFunction -> transitionFunction ? transformFunction

deleteActivePair oldActivePairs activePairToDelete =
if leftInVec `xor` rightInVec
then newActivePairs
else error "In active pairs map should be exact one address"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that message is informative enough. Even more, I'm not sure that I realize what it means.

lamagraph-core/src/Core/MemoryManager.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/MemoryManager.hs Outdated Show resolved Hide resolved
lamagraph-core/src/Core/MemoryManager.hs Outdated Show resolved Hide resolved
<*> signalLoadedNode
<*> (view (containedNode . primaryPort) <$> signalLoadedNode)
<*> pure Primary
in ifoldl
Copy link
Member

Choose a reason for hiding this comment

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

Does associativity matter? Can fold be used?

Copy link
Member

@gsvgit gsvgit left a comment

Choose a reason for hiding this comment

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

First part.

(Eps, _) -> epsToAnyRule lNode rNode
(_, Eps) -> epsToAnyRule rNode lNode
(Apply, Abstract) -> applyToLambdaRule lNode rNode
(Abstract, Apply) -> applyToLambdaRule lNode rNode
Copy link
Member

Choose a reason for hiding this comment

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

Should be left and right node be switched?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is symmetrical

@@ -87,9 +87,12 @@ library
import: common-options
hs-source-dirs: src
exposed-modules:
Core.Core
Copy link
Member

Choose a reason for hiding this comment

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

Is Core' a good name for top-level ? May we use something like ReductionMachine` for top-level namespace?

import Core.Reducer (ChooseReductionRule, getInterface, reducer)

core ::
forall portsNumber nodesNumber edgesNumber cellsNumber dom.
Copy link
Member

Choose a reason for hiding this comment

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

Can core be parametrized with single dom? What about different domains for memory, reducer, other parts?

, HiddenClockResetEnable dom
, Enum AddressNumber
) =>
Vec cellsNumber (Maybe (Node portsNumber)) -> -- Initial network
Copy link
Member

Choose a reason for hiding this comment

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

Not a network, but active pairs. We need (at least) two layers: program loader and processor itself.


3. Written by hands for tests
-}
module Core.Concrete.ReduceRulesLambda where
Copy link
Member

Choose a reason for hiding this comment

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

Not Concrete, but Example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea that in common case this module will generate automatically by Template Haskell and some dsl. It is just example for now, but something more complex in the future

__Note__ this rule is not symmetric!! Erasure node is first
-}
eraseToAbstractOrApplyRule ::
Vec 2 (Maybe AddressNumber) ->
Copy link
Member

Choose a reason for hiding this comment

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

Why Maybe AddressNumber. All checks should be outside the logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without Maybe we need to change Vec size dynamically. It is impossible or very difficult. Or we should allocate a constant amount of memory, even if we don't need it. It is strange

let f = Just (view originalAddress <$> signalLoadedNode, Just . view containedNode <$> signalLoadedNode)
in ram (pure 0) (traverse bundle f)

removeActivePairFromRam ::
Copy link
Member

Choose a reason for hiding this comment

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

Should it be here, not in Ram module?

-- | Data to accumulate all `Port` changes of the `Node`
data Changes (portsNumber :: Nat)
= Changes
{ _secP :: Vec portsNumber (Maybe (Connection portsNumber))
Copy link
Member

Choose a reason for hiding this comment

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

Port, not just P

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.

3 participants