| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4
implement LSCA #8
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for the PR, @isovector! The CI in this repo had bitrotted and was using a version of Ubuntu that GitHub Actions no longer supports. I've pushed a fix in 7e6e07e (and dropped support for some particularly ancient GHC versions in the process). Could you rebase your work on top of the |
|
Done, thanks @RyanGlScott ! |
RyanGlScott
left a comment
RyanGlScott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks reasonable, although there are some CI failures due to the doctests failing to compile.
isovector
force-pushed
the
lsca
branch
2 times, most recently
from
eb66f59 to
f181878
Compare
|
I looked into the CI failures. I believe the reason this happens is because diff --git a/src/Data/LSCA.hs b/src/Data/LSCA.hs
index 59fa80d..c3e73da 100644
--- a/src/Data/LSCA.hs
+++ b/src/Data/LSCA.hs
@@ -41,6 +41,16 @@ import Data.Function (fix)
import Data.Maybe (fromJust)
+-- $setup
+-- >>> :{
+-- g = mkLSAT $ Data.Map.fromList
+-- [ (0, Data.Set.fromList [])
+-- , (1, Data.Set.fromList [0])
+-- , (2, Data.Set.fromList [0])
+-- , (3, Data.Set.fromList [1, 2])
+-- , (4, Data.Set.fromList [2])
+-- ]
+-- :}
-- | A Lowest Single Ancestor Tree, capable of performing fast 'lsa' and 'lsca'
-- queries |
| -- | ||
| -- which is preprocessed into an 'LSAT' via: | ||
| -- | ||
| -- $setup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, a $setup block needs to be a standalone comment with $setup as the first line, or else cabal-docspec won't recognize it. (See the diff in #8 (comment) for an example of what cabal-docspec is searching for.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience with me! I don't have a working setup on my end and am trying to avoid doing so. The challenge afaict is that we'd also like this setup code to be part of the visible haddock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you're suggesting that we copy and paste the initialization in both a $setup and the actual haddock? Which seems reasonable but kinda defeats the idea of a doctest imo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The challenge afaict is that we'd also like this setup code to be part of the visible haddock.
I don't think cabal-docspec allows this. (Unless I have overlooked something, @phadej?)
Unless you're suggesting that we copy and paste the initialization in both a $setup and the actual haddock?
That might be the most expedient way to make this work.
|
Labels
3 participants
This PR implements the "lowest single ancestor" and "lowest single common ancestor" on DAGs, as given by https://www.sciencedirect.com/science/article/abs/pii/S0020019010000487.
I'm not sure it necessarily belongs in this package, but it seems like an obvious schelling point.