Update DataEcon to v0.3 #46

bbejanov · 2023-09-07T13:29:15Z

New version of DataEcon introduces functions to pack and unpack dates of different frequencies given year-period or year-month-day.

codecov-commenter · 2023-09-08T13:57:45Z

Codecov Report

Merging #46 (f452ae4) into dev (b6bdcbb) will increase coverage by 0.17%.
The diff coverage is 93.15%.

@@            Coverage Diff             @@
##              dev      #46      +/-   ##
==========================================
+ Coverage   92.06%   92.24%   +0.17%     
==========================================
  Files          20       20              
  Lines        2785     2901     +116     
==========================================
+ Hits         2564     2676     +112     
- Misses        221      225       +4

Files	Coverage Δ
src/dataecon/C.jl	`93.58% <95.00%> (+3.26%)`	⬆️
src/momentintime.jl	`91.04% <90.90%> (-0.18%)`	⬇️
src/dataecon/DataEcon.jl	`96.27% <95.45%> (-3.73%)`	⬇️
src/dataecon/I.jl	`93.65% <92.36%> (+4.76%)`	⬆️

... and 3 files with indirect coverage changes

YPFrequency not supported yet.

jasonjensen

Thank you for this @bbejanov . DataEcon has really expanded in its functionality!

I only have a few comments and some questions stemming mostly from my own curiosity.

jasonjensen · 2023-10-29T18:36:17Z

src/dataecon/C.jl

+const DE_VER_REVISION = 1
+
+const DE_VER_SUBREVISION = 0
+


Since these are all hard-coded, would it be better if they were derived or do we need some check to ensure consistency here?

This C.jl file is generated using Clang.jl package from the daec.h file in DataEcon. The idea is that the .h file has these constants hard-coded, and there is also a function in the library that returns the same version number (that's de_version()). At run time the user should compare the hardcoded constants in the .h file they are using to the value returned by the library function to make sure they match. For this Julia connector, this check is done in DataEcon.jl in __init__()

jasonjensen · 2023-10-29T18:50:41Z

src/dataecon/DataEcon.jl

+    version = VersionNumber(unsafe_string(C.de_version()))
+    if version != VersionNumber(C.DE_VERSION)
+        throw(ErrorException("Library version $(version) does not match expected version $(C.DE_VERSION)."))
+    end


We may want to specify in the error message that it is the DE library version causing the error here (as opposed to any number of other libraries the user may be loading).

Good idea - I'll update the error message.

jasonjensen · 2023-10-29T18:53:27Z

src/dataecon/DataEcon.jl

    fname = string(fname)
-    I._check(C.de_open(fname, handle))
+    handle = _do_open(readonly ? C.de_open_readonly : C.de_open, fname)


I would set readonly to true by default; I think most open operations will be to get data and load it into memory; we wouldn't want to lock the file unnecessarily. Maybe change the option to write=false?

jasonjensen · 2023-10-29T19:50:41Z

src/dataecon/DataEcon.jl

+# import ..LittleDict
+
+"""
+    get_all_attributes(de, obejct_id)


jasonjensen · 2023-10-29T20:10:29Z

src/dataecon/DataEcon.jl

+Retrieve the names and value of all attributes of the object with the given id.
+They are returned in a dictionary.
+"""
+function get_all_attributes(de::DEFile, id::C.obj_id_t; delim="‖")


So the SQLITE call is just getting the columns and concatenating them with this delimiter, and then we on our side are then splitting the resulting string by this delimiter? Is this the best way to do this? I'm guessing there won't be many attributes, so it can't be that expensive, it just seems a little roundabout.

Here is the query that actually runs in de_get_all_attributes():

So, yes, it concatenates all names in one string with the given delimiter and all the values in the same order as the names and with the same delimiter in another string, and it also returns the number of attributes as a separate value. The main point here is that this query returns at most one row. It may be less efficient than simply SELECT `name`, `value` FROM `attributes` WHERE `id` = ?, but:

the simple query requires a more complex logic to be coded by the caller -- they'd have to write a loop and additional logic for all corner cases. Compare with I._list_catalog() to see what the user would have to do to iterate the rows of a table returned by SQL.

the simple query doesn't distinguish between an object that doesn't exist and an object that exists, but doesn't have any attributes - both cases return a table with 0 rows. The other query returns 0 rows if object doesn't exist and 1 row (with count=0, names="", values="") if object exists but doesn't have any attributes.

Btw, the case of 0-rows is handled by the library - de_get_all_attributes returns an error code (DE_OBJ_DNE), which here in the Julia connector is handled by I._check() to throw an exception. So the end user doesn't even have to check for that.

jasonjensen · 2023-10-29T20:17:14Z

src/dataecon/DataEcon.jl

+    all_names = String[split(unsafe_string(names[]), delim);]
+    all_values = String[split(unsafe_string(values[]), delim);]
+    if length(all_names) != length(all_values)
+        error("number of names and values don't match. Try a different delimiter.")


My initial thought was that this error message doesn't seem that useful since the user is unlikely to call get_all_attributes themselves, but it seems that this is not the case; i.e. get_all_attributes is not called anywhere and is currently there for the sake of completeness?

Correct: get_all_attributes is not used in the connector. It is made available for the end user to call, if they need to.

The idea is that, as an end user, if you want the value of a specific attribute, you call get_attribute. If you want to explore what attributes a given object has, you call get_all_attributes. Setting attributes can only be done one at a time with set_attribute.

jasonjensen · 2023-10-29T20:50:09Z

src/dataecon/I.jl

@@ -56,16 +56,41 @@ _check(rc::Cint) = rc == 0 || throw(DEError())

 const StrOrSym = Union{Symbol,AbstractString}

-_to_de_scalar_val(value) = throw(ArgumentError("Unable to write value of type $(typeof(value))."))
+_to_de_scalar_val(value) = throw(ArgumentError("Unable to write scalar value of type $(typeof(value))."))


Given the pattern of some of your other code, do we want an @nospecialize here?

jasonjensen · 2023-10-29T21:28:41Z

src/dataecon/I.jl

 #############################################################################
 # write tseries and mvtseries

+
+_de_array_data(; eltype, elfreq=C.freq_none, obj_type, nbytes, val) = (; eltype, elfreq, obj_type, nbytes, val)


How does this work? It takes a number of keyword arguments (supplied in any order) and returns an array of their values in a specific order?

:-) this is not my best moment :-)

The logic here is that all methods of _to_de_array would return a sturcture with all relevant fields, which are then used in _do_store_array. But instead of defining an actual struct, I am using a NamedTuple (sort of like an anonymous struct, if you will). This particular function, _de_array_data, acts as "constructor" - so you call it with keyword arguments and it constructs the NamedTuple for you.

This was easier to work with while prototyping, because Revise.jl doesn't handle changes in a struct - you have to restart the repl.

jasonjensen · 2023-10-29T21:43:58Z

src/dataecon/I.jl

-_to_de_array(::Val{true}, value::AbstractMatrix{ET}) where {ET} = (; eltype=_to_de_scalar_type(ET), type=C.type_matrix, nbytes=0, val=C_NULL)
+_to_de_array(::Val{true}, value::AbstractVecOrMat{ET}) where {ET} = _de_array_data(;
+    _eltypefreq(ET)...,
+    obj_type=ndims(value) == 1 ? C.type_vector : C.type_matrix,


TIL: a matrix in Julia is a strictly two-dimensional object.

@jasonjensen

thanks @jasonjensen!

bbejanov · 2023-10-30T19:08:47Z

Thanks @jasonjensen for your review!

tests to compare MIT codes to pack/unpack codes

bc4c69a

bbejanov changed the base branch from master to dev September 7, 2023 13:29

bbejanov marked this pull request as draft September 7, 2023 13:31

bbejanov self-assigned this Sep 7, 2023

DataEcon_jll v0.2.4

3eb3063

bbejanov changed the title ~~Update DataEcon to v0.2.3~~ Update DataEcon to v0.2.4 Sep 8, 2023

bbejanov added 2 commits September 8, 2023 11:33

Construct MIT from Dates.Date.

ae21aac

YPFrequency not supported yet.

Use DataEcon pack/unpack for MIT.

514ac11

bbejanov changed the title ~~Update DataEcon to v0.2.4~~ Update DataEcon to v0.3 Oct 3, 2023

bbejanov and others added 6 commits October 3, 2023 20:02

update to DataEcon 0.3.1 (adding elfreq to series)

0b314a0

list_catalog + catalog_size

25a424d

Merge branch 'dev' into dataecon

4fd06ec

tests for list_catalog, daecopen readonly and daecopenmem

6cb43bd

tests for writing/not writing attributes (jtype, jeltype)

fbf23c8

additional tests

f452ae4

bbejanov marked this pull request as ready for review October 12, 2023 04:50

bbejanov requested a review from jasonjensen October 27, 2023 14:56

bbejanov added 2 commits October 27, 2023 11:20

v

aa85f69

fix typo in error message

88d50fb

jasonjensen approved these changes Oct 29, 2023

View reviewed changes

bbejanov added 2 commits October 30, 2023 13:23

Avoid calling Date(::String) constructor

a599838

fixes from review comments

56d2a0c

thanks @jasonjensen!

bbejanov merged commit cc2268f into dev Oct 30, 2023
6 checks passed

bbejanov deleted the dataecon branch October 30, 2023 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update DataEcon to v0.3 #46

Update DataEcon to v0.3 #46

bbejanov commented Sep 7, 2023 •

edited

Loading

codecov-commenter commented Sep 8, 2023 •

edited

Loading

jasonjensen left a comment

jasonjensen Oct 29, 2023

bbejanov Oct 30, 2023

jasonjensen Oct 29, 2023

bbejanov Oct 30, 2023

jasonjensen Oct 29, 2023

bbejanov Oct 30, 2023

jasonjensen Oct 29, 2023

bbejanov Oct 30, 2023

jasonjensen Oct 29, 2023

bbejanov Oct 30, 2023

jasonjensen Oct 29, 2023

bbejanov Oct 30, 2023

jasonjensen Oct 29, 2023

bbejanov Oct 30, 2023

jasonjensen Oct 29, 2023

bbejanov Oct 30, 2023 •

edited

Loading

jasonjensen Oct 29, 2023

bbejanov commented Oct 30, 2023

Update DataEcon to v0.3 #46

Update DataEcon to v0.3 #46

Conversation

bbejanov commented Sep 7, 2023 • edited Loading

codecov-commenter commented Sep 8, 2023 • edited Loading

Codecov Report

jasonjensen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbejanov Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbejanov commented Oct 30, 2023

bbejanov commented Sep 7, 2023 •

edited

Loading

codecov-commenter commented Sep 8, 2023 •

edited

Loading

bbejanov Oct 30, 2023 •

edited

Loading